Skip to content

feat: add creating directory through context menu in navigation tree #958

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 13 commits into from
Jul 4, 2024

Conversation

walborn
Copy link
Contributor

@walborn walborn commented Jun 28, 2024

closes #915

@walborn walborn requested a review from Raubzeug June 28, 2024 13:56
@Raubzeug
Copy link
Contributor

Press "Enter" does't confirm creating a directory.

</div>
<div>{`${parent}/`}</div>
</div>
<TextInput
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please clarify with Xeno if we need any validation here?

const handleCreateDirectorySubmit = (child: string) => {
createDirectory({database: parent, path: `${parent}/${child}`});

onActivePathUpdate(`${parent}/${child}`);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems we should do it after directory is created (wait for createDirectory answer).


onActivePathUpdate(`${parent}/${child}`);

setCreateDirectoryOpen(false);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here. I would set some loading state to dialog Apply buttons and close dialog on request success. If request fails we may show an error inside the dialog's body.

@import '../../../../styles/mixins.scss';

.ydb-schema-create-directory-dialog {
&__modal {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems this class is redundant.

@Raubzeug
Copy link
Contributor

Raubzeug commented Jul 1, 2024

One more - we should disable "Apply" button, if directory name is empty.

Copy link
Collaborator

@ValeraS ValeraS left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please try not using UseEffect, all current uses are redundant.

React.useEffect(() => {
if (createDirectoryResult.isError) {
const errorMessage =
(createDirectoryResult.error as {data: string})?.data || ' Unknown error';
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does the error contain a message?

Copy link
Contributor Author

@walborn walborn Jul 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The error contains only such fields

{
    "data": "{ status: SCHEME_ERROR, issues: { <main>: Error: Check failed: path: '/local/.sys_health/±', error: symbol '?' is not allowed in the path part '±' } }",
    "status": 400,
    "statusText": "Bad Request",
    "headers": {
        "content-length": "140",
        "content-type": "text/plain"
    },
    "config": {
        "transitional": {
            "silentJSONParsing": true,
            "forcedJSONParsing": true,
            "clarifyTimeoutError": false
        },
        "adapter": [
            "xhr",
            "http"
        ],
        "transformRequest": [
            null
        ],
        "transformResponse": [
            null
        ],
        "timeout": 60000,
        "xsrfCookieName": "",
        "xsrfHeaderName": "X-XSRF-TOKEN",
        "maxContentLength": -1,
        "maxBodyLength": -1,
        "env": {},
        "headers": {
            "Accept": "application/json, text/plain, */*",
            "Content-Type": "application/json"
        },
        "withCredentials": false,
        "signal": {},
        "method": "post",
        "url": "http://localhost:8765/scheme/directory",
        "data": "{}",
        "params": {
            "database": "/local/.sys_health",
            "path": "/local/.sys_health/±"
        },
        "axios-retry": {
            "retries": 3,
            "shouldResetTimeout": false,
            "validateResponse": null,
            "retryCount": 0,
            "lastRequestTime": 1719906437941
        }
    },
    "request": {}
}

<div className={b('description')}>{i18n('schema.tree.dialog.description')}</div>
<div>{`${parentPath}/`}</div>
</div>
<TextInput
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's wrap it to form and call validation and mutation on submit.

error={hasValidationError}
/>
<div className={b('error-wrapper')}>
{hasValidationError && <ResponseError error={validationError} />}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is this not in the TextInput's errorMessage?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To prevent dialog's height jumping.

textButtonApply={i18n('schema.tree.dialog.buttonApply')}
textButtonCancel={i18n('schema.tree.dialog.buttonCancel')}
onClickButtonCancel={handleClose}
propsButtonApply={{disabled: hasValidationError, type: 'submit'}}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
propsButtonApply={{disabled: hasValidationError, type: 'submit'}}
propsButtonApply={{type: 'submit'}}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You suggest not to disable Apply button at all? But why?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do you want to disable it? There is a validation before submission.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it's validation error, we 100% know, that "Apply" won't work.
Or maybe I just like to forbid :)

}

export function CreateDirectoryDialog({open, onClose, parentPath, onSuccess}: SchemaTreeProps) {
const [error, setError] = React.useState<unknown>('');
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this needed? response contains error.

autoFocus
hasClear
disabled={response.isLoading}
error={hasValidationError}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

error prop is deprecated.

@@ -26,14 +28,14 @@ function validateRelativePath(value: string) {
}

export function CreateDirectoryDialog({open, onClose, parentPath, onSuccess}: SchemaTreeProps) {
const [error, setError] = React.useState<unknown>('');
const [showResponseError, setShowResponseError] = React.useState(false);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

response has a reset method

@@ -78,27 +79,29 @@ export function CreateDirectoryDialog({open, onClose, parentPath, onSuccess}: Sc
}}
>
<Dialog.Body>
<div className={b('label')}>
<div className={b('description')}>
<label htmlFor={relativePathInputName} className={b('label')}>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for attribute should be connected to an id of input not a name.

@Raubzeug Raubzeug force-pushed the feat/createDirectory-915 branch from 90c6b6b to 064ac63 Compare July 4, 2024 11:45
@Raubzeug Raubzeug merged commit d1902d4 into main Jul 4, 2024
4 checks passed
@Raubzeug Raubzeug deleted the feat/createDirectory-915 branch July 4, 2024 11:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add create folder action to database/folder context menu
3 participants