-
Notifications
You must be signed in to change notification settings - Fork 8.5k
[File upload] Redesign of the file upload section of the lookup join flyout #244550
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
[File upload] Redesign of the file upload section of the lookup join flyout #244550
Conversation
…elastic/kibana into look-up-join-file-upload-redesign
…elastic/kibana into look-up-join-file-upload-redesign
…elastic/kibana into look-up-join-file-upload-redesign
peteharverson
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested and LGTM
sddonne
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
.../packages/shared/file-upload/src/file_upload_component/new/mapping_editor/mapping_editor.tsx
Outdated
Show resolved
Hide resolved
…elastic/kibana into look-up-join-file-upload-redesign
| return; | ||
| } | ||
| pipeline.processors.push({ | ||
| pipeline.processors!.push({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Maybe we could get rid of this non-null assertion by also checking for the pipeline.processors in the if above?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or perhaps processors can be non-optional in x-pack/platform/packages/shared/file-upload-common/src/types.ts? I saw some more non-null assertions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated in 8ab0e64
| {i18n.translate('xpack.fileUpload.mappingEditor.fieldNameLabel', { | ||
| defaultMessage: 'Field name', | ||
| })} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| {i18n.translate('xpack.fileUpload.mappingEditor.fieldNameLabel', { | |
| defaultMessage: 'Field name', | |
| })} | |
| <FormattedMessage {...} />``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated in 8ab0e64
| {i18n.translate('xpack.fileUpload.mappingEditor.fieldTypeLabel', { | ||
| defaultMessage: 'Field type', | ||
| })} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| {i18n.translate('xpack.fileUpload.mappingEditor.fieldTypeLabel', { | |
| defaultMessage: 'Field type', | |
| })} | |
| <FormattedMessage {...} />``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated in 8ab0e64
| useEffect(() => { | ||
| if (uploadStatus.overallImportStatus === STATUS.COMPLETED) { | ||
| setStep('finish', STATUS.COMPLETED); | ||
| } | ||
| }, [uploadStatus.overallImportStatus, setStep]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| useEffect(() => { | |
| if (uploadStatus.overallImportStatus === STATUS.COMPLETED) { | |
| setStep('finish', STATUS.COMPLETED); | |
| } | |
| }, [uploadStatus.overallImportStatus, setStep]); | |
| const isFinished = uploadStatus.overallImportStatus === STATUS.COMPLETED; |
It would mean removing the finish step from the stepsStatus state, but we could get rid of the useEffect.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated in 8ab0e64
| useEffect(() => { | ||
| setFileUploadActive(true); | ||
| return () => { | ||
| setFileUploadActive(false); | ||
| }; | ||
| }, [setFileUploadActive]); | ||
|
|
||
| useEffect(() => { | ||
| if (filesStatus.length === 0) { | ||
| setFileUploadActive(false); | ||
| } | ||
| }, [filesStatus, setFileUploadActive]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we could combine it into one useEffect such as
| useEffect(() => { | |
| setFileUploadActive(true); | |
| return () => { | |
| setFileUploadActive(false); | |
| }; | |
| }, [setFileUploadActive]); | |
| useEffect(() => { | |
| if (filesStatus.length === 0) { | |
| setFileUploadActive(false); | |
| } | |
| }, [filesStatus, setFileUploadActive]); | |
| useEffect(() => { | |
| setFileUploadActive(filesStatus.length > 0); | |
| return () => { | |
| setFileUploadActive(false); | |
| }; | |
| }, [deps]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated in 8ab0e64
| ? uploadStatus.allDocsSearchable === true | ||
| ? STATUS.COMPLETED | ||
| : STATUS.STARTED | ||
| : stepsStatus.finish |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this might require an update after the recent changes, no? As the stepsStatus.finish would be always STATUS.NOT_STARTED.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've reverted this change d836641
To avoid introducing any last minute bugs
…elastic/kibana into look-up-join-file-upload-redesign
💛 Build succeeded, but was flaky
Failed CI StepsMetrics [docs]Module Count
Public APIs missing comments
Async chunks
Page load bundle
Unknown metric groupsAPI count
async chunk count
ESLint disabled line counts
Total ESLint disabled count
History
|
Remakes the file upload section of the lookup join flyout. https://github.com/user-attachments/assets/c50085d0-b4ea-47ce-8830-8fd0c509960f --------- Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
Remakes the file upload section of the lookup join flyout. https://github.com/user-attachments/assets/c50085d0-b4ea-47ce-8830-8fd0c509960f --------- Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
Remakes the file upload section of the lookup join flyout.
2025-12-02.13-58-34.2025-12-02.13_59_31.mp4