-
Notifications
You must be signed in to change notification settings - Fork 917
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] Add data source aggregated view to show all compatible data sources or only show used data sources #6129
[Multiple Datasource] Add data source aggregated view to show all compatible data sources or only show used data sources #6129
Conversation
…r only used data sources Signed-off-by: Lu Yu <[email protected]>
Signed-off-by: Lu Yu <[email protected]>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6129 +/- ##
==========================================
+ Coverage 67.20% 67.22% +0.01%
==========================================
Files 3333 3334 +1
Lines 64565 64602 +37
Branches 10391 10402 +11
==========================================
+ Hits 43394 43430 +36
+ Misses 18638 18635 -3
- Partials 2533 2537 +4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
...rce_management/public/components/data_source_aggregated_view/data_source_aggregated_view.tsx
Outdated
Show resolved
Hide resolved
...rce_management/public/components/data_source_aggregated_view/data_source_aggregated_view.tsx
Outdated
Show resolved
Hide resolved
...rce_management/public/components/data_source_aggregated_view/data_source_aggregated_view.tsx
Outdated
Show resolved
Hide resolved
...rce_management/public/components/data_source_aggregated_view/data_source_aggregated_view.tsx
Outdated
Show resolved
Hide resolved
...rce_management/public/components/data_source_aggregated_view/data_source_aggregated_view.tsx
Outdated
Show resolved
Hide resolved
...rce_management/public/components/data_source_aggregated_view/data_source_aggregated_view.tsx
Show resolved
Hide resolved
...rce_management/public/components/data_source_aggregated_view/data_source_aggregated_view.tsx
Outdated
Show resolved
Hide resolved
...rce_management/public/components/data_source_aggregated_view/data_source_aggregated_view.tsx
Outdated
Show resolved
Hide resolved
...rce_management/public/components/data_source_aggregated_view/data_source_aggregated_view.tsx
Show resolved
Hide resolved
...rce_management/public/components/data_source_aggregated_view/data_source_aggregated_view.tsx
Outdated
Show resolved
Hide resolved
Signed-off-by: Lu Yu <[email protected]>
Signed-off-by: Lu Yu <[email protected]>
Signed-off-by: Lu Yu <[email protected]>
} | ||
}) | ||
.catch(() => { | ||
this.props.notifications.addWarning( |
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 catch warning notifications here, but we didn't change the component state, do we need to set state to reflect the error condition?
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.
If there is error, this will show a toast about the error
if (!showDataSourceSelectable && !showDataSourceView && !showDataSourceAggregatedView) { | ||
return null; | ||
} |
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.
These 3 boolean values are used to control the appearance of this component. If it was just one variable, it would have been acceptable but anything more than one variable controlling how exclusively something is rendered is wrong. This has to be changed to a single ENUM variable.
PS: this piece of code cannot remain in the repo as is.
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 comment! Will address this in #6239 if the proposed change looks good @AMoo-Miki
…patible data sources or only show used data sources (#6129) * add data source aggregated view to show all compatible data sources or only used data sources Signed-off-by: Lu Yu <[email protected]> * add change log Signed-off-by: Lu Yu <[email protected]> * address comments and add more tests Signed-off-by: Lu Yu <[email protected]> --------- Signed-off-by: Lu Yu <[email protected]> (cherry picked from commit 05abf5e) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> # Conflicts: # CHANGELOG.md
…patible data sources or only show used data sources (#6129) (#6264) * add data source aggregated view to show all compatible data sources or only used data sources Signed-off-by: Lu Yu <[email protected]> * add change log Signed-off-by: Lu Yu <[email protected]> * address comments and add more tests Signed-off-by: Lu Yu <[email protected]> --------- Signed-off-by: Lu Yu <[email protected]> (cherry picked from commit 05abf5e) 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
This change is based on requirement to show readonly view for all compatible connected data sources in the navigation bar, and also is capable of showing only the data sources passed by the plugins
Issues Resolved
Screenshot
readonlyaggregatedview.mp4
Testing the changes
The following were performed in the recording above:
SELECTED DATA SOURCES
, the button should show the number of active/used data sourcesDATA SOURCES
followed by number of compatible data sources and display them.Check List
yarn test:jest
yarn test:jest_integration