-
Notifications
You must be signed in to change notification settings - Fork 855
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
[Multiple Datasource] Refactor data source menu and interface to allow cleaner selection of component and related configurations #6256
Conversation
Signed-off-by: Lu Yu <nluyu@amazon.com>
Signed-off-by: Lu Yu <nluyu@amazon.com>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6256 +/- ##
==========================================
+ Coverage 67.34% 67.35% +0.01%
==========================================
Files 3352 3353 +1
Lines 65082 65083 +1
Branches 10486 10483 -3
==========================================
+ Hits 43831 43839 +8
+ Misses 18699 18694 -5
+ Partials 2552 2550 -2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Signed-off-by: Lu Yu <nluyu@amazon.com>
return null; | ||
} | ||
export function DataSourceMenu<T>(props: DataSourceMenuProps<T>): ReactElement | null { | ||
const { componentType, componentConfig } = props; |
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.
Any reason why we cannot simply made the type prop be type
?
Also, is there a reason to have a bunch of props clubbed into config
? To me, it feels natural not club them.
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.
@AMoo-Miki , this was based on discussion with @ashwin-pc @Flyingliuhub @ZilongX @yujin-emma last week to group the configs specific to a component and use generic type for better readability which I think makes a lot of sense here. If we don't do that, all the configs to all components will need to the the prop for Data Source Menu. Also, this is similar to how TopNavMenu accepts the config https://github.com/opensearch-project/OpenSearch-Dashboards/blob/main/src/plugins/navigation/public/top_nav_menu/top_nav_menu.tsx#L47
src/plugins/data_source_management/public/components/data_source_menu/types.ts
Outdated
Show resolved
Hide resolved
setMenuMountPoint?: (menuMount: MountPoint | undefined) => void; | ||
componentConfig: T; |
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.
same as above. Just for a sake of readability I guess
@@ -63,6 +63,7 @@ Inspired from [Keep a Changelog](https://keepachangelog.com/en/1.0.0/) | |||
- [Workspace] Register a workspace dropdown menu at the top of left nav bar ([#6150](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/6150)) | |||
- [Multiple Datasource] Add icon in datasource table page to show the default datasource ([#6231](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/6231)) | |||
- [Multiple Datasource] Add TLS configuration for multiple data sources ([#6171](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/6171)) | |||
- [Multiple Datasource] Refactor data source menu and interface to allow cleaner selection of component and related configurations ([#6256](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/6256)) |
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.
Just a small trick to avoid potential changelog conflicts (since there are a lot..), instead of attaching the changelog entry as last line (L66 in this case), simply stuff it into the middle (say L64) would save the efforts to resolve changelog conflicts
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.
Good to know, thanks!
|
||
interface DataSourceSelectableProps { | ||
savedObjectsClient: SavedObjectsClientContract; | ||
notifications: ToastsStart; | ||
onSelectedDataSource: (dataSource: DataSourceOption) => void; | ||
onSelectedDataSources: (dataSources: DataSourceOption[]) => void; |
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.
any context of the renaming from onSelectedDataSource
to onSelectedDataSources
? MDS drop down doesn't support multiple selected right ? (aka we can only select one data source at a time)
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.
We are supporting multi select which is in another PR, thus changing it to selected data sources
Signed-off-by: Lu Yu <nluyu@amazon.com>
5a9206f
…w cleaner selection of component and related configurations (#6256) * refactor Signed-off-by: Lu Yu <nluyu@amazon.com> * refactor data source menu Signed-off-by: Lu Yu <nluyu@amazon.com> * add change log Signed-off-by: Lu Yu <nluyu@amazon.com> * move configs based on if it is required Signed-off-by: Lu Yu <nluyu@amazon.com> * move optional params below required Signed-off-by: Lu Yu <nluyu@amazon.com> --------- Signed-off-by: Lu Yu <nluyu@amazon.com> Co-authored-by: ZilongX <99905560+ZilongX@users.noreply.github.com> (cherry picked from commit 4f1884b) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> # Conflicts: # CHANGELOG.md
…w cleaner selection of component and related configurations (#6256) (#6267) * refactor Signed-off-by: Lu Yu <nluyu@amazon.com> * refactor data source menu Signed-off-by: Lu Yu <nluyu@amazon.com> * add change log Signed-off-by: Lu Yu <nluyu@amazon.com> * move configs based on if it is required Signed-off-by: Lu Yu <nluyu@amazon.com> * move optional params below required Signed-off-by: Lu Yu <nluyu@amazon.com> --------- Signed-off-by: Lu Yu <nluyu@amazon.com> Co-authored-by: ZilongX <99905560+ZilongX@users.noreply.github.com> (cherry picked from commit 4f1884b) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> # Conflicts: # CHANGELOG.md Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: Lu Yu <nluyu@amazon.com>
Description
This change refactors data source menu and its interface to allow cleaner selection of respective data source component and related configurations
Issues Resolved
Fixes #6239
Screenshot
refactor2.mp4
Testing the changes
The following were performed in the recording:
Check List
yarn test:jest
yarn test:jest_integration