-
Notifications
You must be signed in to change notification settings - Fork 918
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
[MDS] Update Data source selector behavior for defaultOption #6372
[MDS] Update Data source selector behavior for defaultOption #6372
Conversation
Signed-off-by: Huy Nguyen <[email protected]>
Signed-off-by: Huy Nguyen <[email protected]>
Signed-off-by: Huy Nguyen <[email protected]>
Signed-off-by: Huy Nguyen <[email protected]>
Signed-off-by: Huy Nguyen <[email protected]>
Signed-off-by: Huy Nguyen <[email protected]>
Signed-off-by: Huy Nguyen <[email protected]>
Signed-off-by: Huy Nguyen <[email protected]>
...s/data_source_management/public/components/data_source_selectable/data_source_selectable.tsx
Outdated
Show resolved
Hide resolved
...s/data_source_management/public/components/data_source_selectable/data_source_selectable.tsx
Outdated
Show resolved
Hide resolved
i18n.translate('dataSource.fetchDataSourceError', { | ||
defaultMessage: 'Data source with id is not available', | ||
}) | ||
); |
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.
Should we also use the callback function to tell plugins team that Invalid state?
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.
So this function, regardless of valid/invalid id, will set the new state and call the callback function. If the id is valid, we change the selectedOption
to be whatever was set in props and call the callback function with this. IF the id is invalid, we change the selectedOption
to be []
and return the callback function with []
. The only thing that changes is whether to add a toast message.
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.
GOt it.
}); | ||
this.props.onSelectedDataSource(selectedOption); | ||
} |
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.
What if plugins team pass in local cluster?
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.
Local Cluster
should already be in the dataSources
because we check if the hideLocalCluster
is disabled and add the local cluster before calling this method. In this case, the id = ''
and because Local Cluster
is in the list of datasources, dataSources.find((ds) => ds.id === id)
should return the Local Cluster
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.
Make sense!
Signed-off-by: Huy Nguyen <[email protected]>
Signed-off-by: Huy Nguyen <[email protected]>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #6372 +/- ##
===========================================
- Coverage 55.58% 32.63% -22.96%
===========================================
Files 1199 2249 +1050
Lines 24259 45490 +21231
Branches 4087 7127 +3040
===========================================
+ Hits 13485 14844 +1359
- Misses 10133 29951 +19818
- Partials 641 695 +54
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Hi Huy. Could you check the code cov. It shows failed on my side |
if (!dataSource) { | ||
this.props.notifications.addWarning( | ||
i18n.translate('dataSource.fetchDataSourceError', { | ||
defaultMessage: 'Data source with id is not available', |
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 probably should start extracting the error handling messages out so that we can keep the message consistent across all data source component
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, can you resolve the conflict of changelog.md
Signed-off-by: Huy Nguyen <[email protected]>
* Refactor selector component changes Signed-off-by: Huy Nguyen <[email protected]> * Refactor selector Signed-off-by: Huy Nguyen <[email protected]> * Add unit tests for selector component Signed-off-by: Huy Nguyen <[email protected]> * Set initial state as [] to prevent page breaking Signed-off-by: Huy Nguyen <[email protected]> * Remove change Signed-off-by: Huy Nguyen <[email protected]> * Add changelog Signed-off-by: Huy Nguyen <[email protected]> * Update snapshots Signed-off-by: Huy Nguyen <[email protected]> * Update snapshots and fix tests Signed-off-by: Huy Nguyen <[email protected]> * Fix merge conflict Signed-off-by: Huy Nguyen <[email protected]> --------- Signed-off-by: Huy Nguyen <[email protected]> Co-authored-by: Lu Yu <[email protected]> (cherry picked from commit 8151ef5) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> # Conflicts: # CHANGELOG.md
Thanks @huyaboo @zhyuanqi @BionIT @zhongnansu to keep enhance the experience for MDS |
…6391) * Refactor selector component changes Signed-off-by: Huy Nguyen <[email protected]> * Refactor selector Signed-off-by: Huy Nguyen <[email protected]> * Add unit tests for selector component Signed-off-by: Huy Nguyen <[email protected]> * Set initial state as [] to prevent page breaking Signed-off-by: Huy Nguyen <[email protected]> * Remove change Signed-off-by: Huy Nguyen <[email protected]> * Add changelog Signed-off-by: Huy Nguyen <[email protected]> * Update snapshots Signed-off-by: Huy Nguyen <[email protected]> * Update snapshots and fix tests Signed-off-by: Huy Nguyen <[email protected]> * Fix merge conflict Signed-off-by: Huy Nguyen <[email protected]> --------- Signed-off-by: Huy Nguyen <[email protected]> Co-authored-by: Lu Yu <[email protected]> (cherry picked from commit 8151ef5) 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>
Description
Previously,
DataSourceSelector
did not respect the Placeholder text. This PR will modify the behavior ofdefaultOption
to be more consistentTODO:
DataSourceSelectable
. This will be addressed when [Multiple DataSource] DataSourceSelectable support to render label by getting it from dataSourceOptions #6358 is mergedIssues Resolved
Partially addresses #6274
Screenshot
Testing the changes
The following are the test scenarios:
if
defaultOption = undefined;
:defaultDataSource
:Local Cluster
ifhideLocalCluster
is disabled anddefaultDataSource
is filtered out:Local Cluster
is hidden anddefaultDataSource
is filtered out:if
defaultOption = []
:if
defaultOption = [{id}]
:id = undefined
throw toast:id = Source B
andSource B
is not filtered out, show selected option:id = Source B
andSource B
is filtered out, show toast:id = invalid-id
, show toast:Check List
yarn test:jest
yarn test:jest_integration