-
Notifications
You must be signed in to change notification settings - Fork 397
[ui-importer] destination setting component and hook #4157
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
Conversation
✅ Test files were modified. Ensure that the tests cover all relevant changes. ✅ |
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.
Pull Request Overview
This PR introduces a new DataCatalog hook and a DestinationSettings component while updating the importer file preview logic to utilize these new components for configuring destination settings. Key changes include:
- The addition of a useDataCatalog hook to manage engine, database, and compute selections.
- Integration of the DestinationSettings component in the file preview along with modifications to the finish import logic.
- Minor style adjustments and type updates related to importer file types and destination configuration.
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
File | Description |
---|---|
desktop/core/src/desktop/js/utils/hooks/useDataCatalog/useDataCatalog.tsx | Introduces a new hook for fetching and managing DataCatalog data. |
desktop/core/src/desktop/js/apps/newimporter/types.ts | Updates importer types by converting a const enum to an enum and adding DestinationConfig. |
desktop/core/src/desktop/js/apps/newimporter/ImporterFilePreview/ImporterFilePreview.tsx | Updates finish import logic to utilize destinationConfig and integrates DestinationSettings. |
desktop/core/src/desktop/js/apps/newimporter/ImporterFilePreview/ImporterFilePreview.scss | Removes a fixed height setting from metadata styling. |
desktop/core/src/desktop/js/apps/newimporter/ImporterFilePreview/DestinationSettings/DestinationSettings.tsx | Adds a new component for destination configuration and integrates useDataCatalog usage. |
desktop/core/src/desktop/js/apps/newimporter/ImporterFilePreview/DestinationSettings/DestinationSettings.scss | Provides styling for the new DestinationSettings component. |
desktop/core/src/desktop/js/apps/newimporter/ImporterFilePreview/ImporterFilePreview.tsx
Show resolved
Hide resolved
.../desktop/js/apps/newimporter/ImporterFilePreview/DestinationSettings/DestinationSettings.tsx
Show resolved
Hide resolved
desktop/core/src/desktop/js/utils/hooks/useDataCatalog/useDataCatalog.tsx
Show resolved
Hide resolved
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.
Nice work, but I'd also like to see the following for this PR:
- a screenshot for the new UI part
- unit tests on all new components
...desktop/js/apps/newimporter/ImporterFilePreview/DestinationSettings/DestinationSettings.scss
Outdated
Show resolved
Hide resolved
...desktop/js/apps/newimporter/ImporterFilePreview/DestinationSettings/DestinationSettings.scss
Show resolved
Hide resolved
.../desktop/js/apps/newimporter/ImporterFilePreview/DestinationSettings/DestinationSettings.tsx
Outdated
Show resolved
Hide resolved
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.
Nice work,
just a small UI misalignment I saw in the screenshot. The rightmost textbox does not have the same height as the dropdowns.
What changes were proposed in this pull request?
How was this patch tested?
Please review Hue Contributing Guide before opening a pull request.