[MD]Add default icon in multi-selectable picker #6357

zhyuanqi · 2024-04-05T21:58:53Z

Description

Add default icon in multi-selectable picker and fix a bug in when datasource length is 0, localcluster would not be added.

Screenshot

Screen.Recording.2024-04-05.at.2.55.15.PM.mov

Testing the changes

Check List

All tests pass
- yarn test:jest
- yarn test:jest_integration
New functionality includes testing.
New functionality has been documented.
Update CHANGELOG.md
Commits are signed per the DCO using --signoff

codecov · 2024-04-05T22:14:35Z

Codecov Report

Attention: Patch coverage is 85.71429% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 67.52%. Comparing base (85df662) to head (c92dc71).

❗ Current head c92dc71 differs from pull request most recent head da6b83e. Consider uploading reports for the commit da6b83e to get more accurate results

Files	Patch %	Lines
..._multi_selectable/data_source_multi_selectable.tsx	84.61%	0 Missing and 2 partials ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##             main    #6357       +/-   ##
===========================================
+ Coverage   55.58%   67.52%   +11.93%     
===========================================
  Files        1199     3378     +2179     
  Lines       24259    65885    +41626     
  Branches     4087    10661     +6574     
===========================================
+ Hits        13485    44487    +31002     
- Misses      10133    18808     +8675     
- Partials      641     2590     +1949

Flag	Coverage Δ
Linux_1	`32.63% <ø> (?)`
Linux_2	`55.62% <ø> (?)`
Linux_3	`44.90% <85.71%> (?)`
Linux_4	`34.99% <0.00%> (?)`
Windows_1	`32.65% <ø> (?)`
Windows_2	`55.58% <ø> (ø)`
Windows_3	`44.91% <85.71%> (?)`
Windows_4	`34.99% <0.00%> (?)`

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

BionIT · 2024-04-07T23:29:25Z

...ource_management/public/components/data_source_multi_selectable/data_source_filter_group.tsx

@@ -148,7 +153,14 @@ export const DataSourceFilterGroup: React.FC<DataSourceFilterGroupProps> = ({
              showIcons={true}
              style={itemStyle}
            >
-              {item.label}
+              <EuiFlexGroup alignItems="center">


Looks like the default icon can be extracted into its own module since it will be shared among multiple components

BionIT · 2024-04-07T23:32:15Z

...e_management/public/components/data_source_multi_selectable/data_source_multi_selectable.tsx

      });
+
+      this.props.onSelectedDataSources(selectedOptions.filter((option) => option.checked === 'on'));


looks like we can just send back selectedOptions since selectedOptions always have checked field being set to on

Will make the change

BionIT · 2024-04-09T01:06:52Z

src/plugins/data_source_management/public/components/data_source_option.tsx

+  defaultDataSource: string | null;
+}
+
+export const ShowDataSourceOption: React.FC<ShowDataSourceOptionProps> = ({


Can we simply name it DataSourceOption and then DataSourceOptionProps

Hi We have a type called DataSourceOption already. Let me rename it to DataSourceOptionItem as it shows each item

zhongnansu · 2024-04-09T17:07:57Z

...e_management/public/components/data_source_multi_selectable/data_source_multi_selectable.tsx

-              visible: true,
-            })
-          );
+    try {


why change from .catch() to try {} catch(){}, I didn't see the code logic differer from before

Hi Zhongnan this is the original code(https://github.com/opensearch-project/OpenSearch-Dashboards/blob/main/src/plugins/data_source_management/public/components/data_source_multi_selectable/data_source_multi_selectable.tsx#L46). It puts all the logic under if (fetchedDataSources?.length) { I want to refactor the code, so i use the try{} catch {}
Now it is

try { const fetchedDataSources = await getDataSourcesWithFields.... if (fetchedDataSources?.length) { ... } if (!this.props.hideLocalCluster) { .... } }.catch() { }

@zhyuanqi got you. this is fixing the bug that local host not able to render due to datasource.length = 0. Thanks!

zhongnansu

lgtm

BionIT · 2024-04-09T18:05:49Z

Tests failed, can you fix https://github.com/opensearch-project/OpenSearch-Dashboards/actions/runs/8611105138/job/23597714580?pr=6357

Signed-off-by: Yuanqi(Ella) Zhu <[email protected]>

Signed-off-by: Yuanqi(Ella) Zhu <[email protected]> (cherry picked from commit 87deeda) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> # Conflicts: # CHANGELOG.md

Signed-off-by: Yuanqi(Ella) Zhu <[email protected]> (cherry picked from commit 87deeda) 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>

zhyuanqi requested review from ananzh, kavilla, AMoo-Miki, ashwin-pc, joshuarrrr, abbyhu2000, zengyan-amazon, zhongnansu, manasvinibs, ZilongX, Flyingliuhub, curq, bandinib-amzn, SuZhou-Joe, ruanyl, BionIT and xinruiba as code owners April 5, 2024 21:58

github-actions bot added the valued-contributor label Apr 5, 2024

zhyuanqi force-pushed the readonly branch from b0c6c81 to c652533 Compare April 5, 2024 22:00

zhyuanqi changed the title ~~Add default icon in multi-selectable picker and fix a bug~~ Add default icon in multi-selectable picker Apr 5, 2024

zhyuanqi force-pushed the readonly branch from c652533 to c92dc71 Compare April 5, 2024 22:12

BionIT reviewed Apr 7, 2024

View reviewed changes

zhyuanqi force-pushed the readonly branch 2 times, most recently from 643e663 to 5924364 Compare April 8, 2024 23:53

BionIT reviewed Apr 9, 2024

View reviewed changes

zhyuanqi force-pushed the readonly branch from 5924364 to a317edd Compare April 9, 2024 06:07

zhongnansu reviewed Apr 9, 2024

View reviewed changes

zhongnansu previously approved these changes Apr 9, 2024

View reviewed changes

zhongnansu changed the title ~~Add default icon in multi-selectable picker~~ [MD]Add default icon in multi-selectable picker Apr 9, 2024

zhongnansu added backport 2.x multiple datasource multiple datasource project labels Apr 9, 2024

BionIT previously approved these changes Apr 9, 2024

View reviewed changes

zhyuanqi dismissed stale reviews from BionIT and zhongnansu via 95268bf April 9, 2024 18:13

zhyuanqi force-pushed the readonly branch 2 times, most recently from 95268bf to d2b8457 Compare April 9, 2024 18:14

Add default icon in multi-selectable picker

da6b83e

Signed-off-by: Yuanqi(Ella) Zhu <[email protected]>

zhyuanqi force-pushed the readonly branch from d2b8457 to da6b83e Compare April 9, 2024 18:17

BionIT approved these changes Apr 9, 2024

View reviewed changes

zhongnansu approved these changes Apr 9, 2024

View reviewed changes

zhongnansu merged commit 87deeda into opensearch-project:main Apr 9, 2024
66 checks passed

opensearch-trigger-bot bot mentioned this pull request Apr 9, 2024

[Backport 2.x] [MD]Add default icon in multi-selectable picker #6383

Merged

BionIT mentioned this pull request Apr 10, 2024

[Meta][UX][Multiple Datasource] Improvement to data source components in the navigation bar #6216

Closed

36 tasks

BionIT added the v2.14.0 label Apr 11, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MD]Add default icon in multi-selectable picker #6357

[MD]Add default icon in multi-selectable picker #6357

zhyuanqi commented Apr 5, 2024 •

edited

Loading

codecov bot commented Apr 5, 2024 •

edited

Loading

BionIT Apr 7, 2024

zhyuanqi Apr 8, 2024

BionIT Apr 7, 2024

zhyuanqi Apr 8, 2024

BionIT Apr 9, 2024

zhyuanqi Apr 9, 2024

zhongnansu Apr 9, 2024

zhyuanqi Apr 9, 2024

zhongnansu Apr 9, 2024

zhongnansu left a comment

BionIT commented Apr 9, 2024

		});

		this.props.onSelectedDataSources(selectedOptions.filter((option) => option.checked === 'on'));

[MD]Add default icon in multi-selectable picker #6357

[MD]Add default icon in multi-selectable picker #6357

Conversation

zhyuanqi commented Apr 5, 2024 • edited Loading

Description

Screenshot

Testing the changes

Check List

codecov bot commented Apr 5, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhongnansu left a comment

Choose a reason for hiding this comment

BionIT commented Apr 9, 2024

zhyuanqi commented Apr 5, 2024 •

edited

Loading

codecov bot commented Apr 5, 2024 •

edited

Loading