-
Notifications
You must be signed in to change notification settings - Fork 1.1k
[workspace]Support scope in data source selector #9832
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
[workspace]Support scope in data source selector #9832
Conversation
c1a6097 to
79bbb9f
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #9832 +/- ##
=======================================
Coverage 61.70% 61.71%
=======================================
Files 4227 4227
Lines 107599 107607 +8
Branches 17474 17477 +3
=======================================
+ Hits 66399 66406 +7
+ Misses 36746 36745 -1
- Partials 4454 4456 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
32c5f48 to
5b021cc
Compare
c27cc59 to
568386c
Compare
| onSelectedDataSource: (dataSourceOption: DataSourceOption[]) => void; | ||
| disabled: boolean; | ||
| fullWidth: boolean; | ||
| getWorkspaces: () => WorkspacesStart; |
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.
Why getWorkspaces was added into this Props, I think we just used it in this component, could we just use getWorkspace function directly and do not need to update src/plugins/data_source_management/public/components/data_source_selector/create_data_source_selector.tsx.
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.
sure, updated, thanks
| import { DataSourceItem } from '../data_source_item'; | ||
| import './data_source_selector.scss'; | ||
| import { DataSourceOption } from '../data_source_menu/types'; | ||
| import { getWorkspaces } from '../utils'; |
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: you can add getWorkspaces at line 23.
| ) { | ||
| const { hideLocalCluster } = dataSourcePluginSetup; | ||
| return (props: DataSourceSelectorProps) => ( | ||
| return (props: Omit<DataSourceSelectorProps, 'getWorkspaces'>) => ( |
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: do not need to use Omit and delete 'getWorkspaces'
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, thanks~
Signed-off-by: Qxisylolo <qianxisy@amazon.com>
Signed-off-by: Qxisylolo <qianxisy@amazon.com>
Signed-off-by: Qxisylolo <qianxisy@amazon.com>
d5da686 to
b3b47d9
Compare
| savedObjects: client, | ||
| notifications, | ||
| }, | ||
| scope: UiSettingScope.WORKSPACE, |
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.
Shall we use Global as default scope?
| }, | ||
| scope: UiSettingScope.WORKSPACE, | ||
| uiSettings, | ||
| hideLocalCluster: false, |
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 should keep hideLocalCluster: true.
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.
oh sorry. I paste the wrong line. updated
| hideLocalCluster, | ||
| application, | ||
| onManageDataSource, | ||
| scope, |
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.
For other plugins that have integrated with MDS, they do not need to manually pass scope right?
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.
yes.
| disabled={false} | ||
| hideLocalCluster={false} | ||
| fullWidth={false} | ||
| scope={UiSettingScope.WORKSPACE} |
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.
Workspace is still a feature that requires feature flag, shall we make it GLOBAL?
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.
ok
Signed-off-by: Qxisylolo <qianxisy@amazon.com>
...ugins/data_source_management/public/components/data_source_selector/data_source_selector.tsx
Show resolved
Hide resolved
Signed-off-by: Qxisylolo <qianxisy@amazon.com>
b562667 to
2a83031
Compare
SuZhou-Joe
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.
Thanks for the comprehensive change.
Description
this pr supports scope in data source selector.
Screenshot
2025-05-30.16.09.47.mov
2025-05-30.16.13.48.mov
Testing the changes
Changelog
Check List
yarn test:jestyarn test:jest_integration