feat: autoSelectDefault and showUsageStatus props of StorageSelector #2180

ironAiken2 · 2024-01-26T07:03:23Z

This PR resolves #2151

feature:

autoSelectDefault
: set default selected host, "default" | "usage" or no use.
showUsageSelect
: "true" for showing usage in each host, "false" for disabling the feature.
useControllableValue
: StorageSelect component can be controllable or not. If the parent passes state, it uses it; otherwise, it uses its own state.

Checklist: (if applicable)

Mention to the original issue
Documentation
Minium required manager version
Specific setting for review (eg., KB link, endpoint or how to setup)
Minimum requirements to check during review
Test case(s) to demonstrate the difference of before/after

…sageStatus cases: 1. all props are "false" or only showUsageStatus is "false : no auto selected value 2. only autoSelectDefault is "true" : use query response's default field 3. autoSelectDefault and showUsageStatus are both "true" : set default value that have minimum usage

aws-amplify-ap-northeast-2 · 2024-01-26T07:08:47Z

This pull request is automatically being deployed by Amplify Hosting (learn more).

Access this pull request here: https://pr-2180.d3g9cs6u59b8lw.amplifyapp.com

react/src/components/StorageSelect.tsx

- no use : select nothing - 'default' : select default value in RQ response - 'usage' : select by minimum usage host

lizable

Works okay, but there are a few touchups needed. Please refer to the comments above.

react/src/components/StorageSelect.tsx

+          <Flex justify="between" align="center">
+            <Badge
+              // @ts-ignore
+              status={status[idx]}


react/src/components/StorageSelect.tsx

+  }, [vhostInfo]);
+
+  const optionRender = useMemo(() => {
+    const status = ['success', 'warning', 'error'];


lizable

I just found some minor test failures in github action, originated from dependency array in hooks(useEffect and useMemo). You could either follow the guidance from github action or just input (// eslint-disable-next-line react-hooks/exhaustive-deps) right above the line if there has certain reason (facebook/create-react-app#6880 (comment)).

lizable

Please resolve test failure on github action following the comments above.

github-actions · 2024-02-01T15:55:02Z

Coverage report for `./react`

St.^❔	Category	Percentage	Covered / Total
🔴	Statements	3.14% (+0.01% 🔼)	101/3214
🔴	Branches	3.32% (+0.01% 🔼)	67/2016
🔴	Functions	1.56% (+0.01% 🔼)	17/1093
🔴	Lines	3.2% (+0.01% 🔼)	101/3154

Show new covered files 🐣

St.^❔	File	Statements	Branches	Functions	Lines
🔴	`...` / StorageSelect.tsx	0%	0%	0%	0%

Test suite run success

20 tests passing in 4 suites.

Report generated by 🧪jest coverage report action from 876731c

ironAiken2 · 2024-02-01T16:01:46Z

I thought the "vhostInfo" data is used directly within the useEffect, and changes in it have a significant impact on determining the selectd host. So I added vhostInfo to the dependencies array.

However, the other items are only configurable in code, so I removed them from the Dependencies array because I thought they were less likely to be changed because the user can't change their settings.

lizable

LGTM.

- Refactoring code by using defaultValue - Add conditional rendering for when usage is not included

yomybaby · 2024-02-06T02:38:39Z

react/src/components/StorageSelect.tsx

+  const [state, setState] = useControllableValue({ value, onChange });
+
+  useEffect(() => {
+    console.log('!');


Please remove this line.

yomybaby · 2024-02-06T02:42:40Z

react/src/components/StorageSelect.tsx

+        minWidth: 165,
+        direction: 'ltr',


Is there any reason to set the direction here?

yomybaby · 2024-02-08T03:03:14Z

react/src/components/StorageSelect.tsx

+            {/* TODO: add tooltip for '여유/주의/부족' */}
+            {vhostInfo?.volume_info[host]?.usage && (
+              <Tooltip
+                title="여유 or 주의 or 부족"
+                // @ts-ignore
+                getPopupContainer={() => shadowRoot}


I push a commit to add a tooltip with getPopupContainer. Please add a proper text for each case. @ironAiken2

yomybaby

LGTM!

yomybaby · 2024-02-08T08:12:32Z

I pushed commits to display search highlights and update type definition. @ironAiken2 , Please check.
I merged this PR. Thank you!

ironAiken2 added 2 commits January 25, 2024 16:44

feat: show usage status indicator

6157b30

github-actions bot assigned ironAiken2 Jan 26, 2024

github-actions bot added the size:XL 500~ LoC label Jan 26, 2024

This comment was marked as resolved.

Sign in to view

ironAiken2 requested a review from yomybaby January 26, 2024 07:13

fix: antd version downgrading into 5.11.0

cca0067

yomybaby requested changes Jan 26, 2024

View reviewed changes

react/src/components/StorageSelect.tsx Outdated Show resolved Hide resolved

react/src/components/StorageSelect.tsx Outdated Show resolved Hide resolved

react/src/components/StorageSelect.tsx Show resolved Hide resolved

react/src/components/StorageSelect.tsx Outdated Show resolved Hide resolved

ironAiken2 added 5 commits January 29, 2024 11:38

feat: set autoSelectType for selecting standard

cce9c88

- no use : select nothing - 'default' : select default value in RQ response - 'usage' : select by minimum usage host

feat: set optional render by host's usage

e9016be

Merge remote-tracking branch 'origin/main' into feat/storage-selector

f7472e8

feat: apply useControllableValue hook

1730d12

fix: version integration with main branch

70178c4

github-actions bot added size:L 100~500 LoC and removed size:XL 500~ LoC labels Jan 29, 2024

feat: add detail info button into select option

7519e6f

ironAiken2 requested a review from lizable January 31, 2024 06:29

lizable requested changes Jan 31, 2024

View reviewed changes

fix: use usageIndicatorColor to select color by usage

a853749

ironAiken2 requested review from yomybaby and lizable February 1, 2024 05:59

lizable approved these changes Feb 1, 2024

View reviewed changes

lizable self-requested a review February 1, 2024 09:07

lizable requested changes Feb 1, 2024

View reviewed changes

feat: add useEffect dependency ignore syntax comment

7a68d36

ironAiken2 force-pushed the feat/storage-selector branch from 794b1e2 to 7a68d36 Compare February 5, 2024 14:27

ironAiken2 requested a review from lizable February 6, 2024 01:57

lizable approved these changes Feb 6, 2024

View reviewed changes

yomybaby and others added 5 commits February 6, 2024 12:35

refactoring

95438f2

refactoring

c55eaf8

- Refactoring code by using defaultValue - Add conditional rendering for when usage is not included

refactor: direct assignment without variable declaration.

577ec54

refactor: delete useless import

9dce36c

add tooltip to badge area only and remove dummy button

c97726d

yomybaby requested changes Feb 8, 2024

View reviewed changes

ironAiken2 and others added 3 commits February 8, 2024 13:41

feat: add text into tooltip for each usage case

ec9920f

feat: showSearch and searchValue highlight

c24f537

add volume_info type definition

876731c

yomybaby approved these changes Feb 8, 2024

View reviewed changes

yomybaby merged commit 1df0f7d into main Feb 8, 2024
6 of 7 checks passed

yomybaby deleted the feat/storage-selector branch February 8, 2024 08:12

ironAiken2 mentioned this pull request Apr 1, 2024

Let's provide unit test code for the StorageSelect. #2287

Open

agatha197 mentioned this pull request Apr 1, 2024

feat: display Quota per storage volume info if quota is available #2285

Merged

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: autoSelectDefault and showUsageStatus props of StorageSelector #2180

feat: autoSelectDefault and showUsageStatus props of StorageSelector #2180

ironAiken2 commented Jan 26, 2024 •

edited by yomybaby

Loading

aws-amplify-ap-northeast-2 bot commented Jan 26, 2024

This comment was marked as resolved.

lizable left a comment

This comment was marked as resolved.

This comment was marked as resolved.

lizable left a comment •

edited

Loading

lizable left a comment

github-actions bot commented Feb 1, 2024 •

edited

Loading

ironAiken2 commented Feb 1, 2024

lizable left a comment

yomybaby Feb 6, 2024

yomybaby Feb 6, 2024

yomybaby Feb 8, 2024

yomybaby left a comment

yomybaby commented Feb 8, 2024

feat: autoSelectDefault and showUsageStatus props of StorageSelector #2180

feat: autoSelectDefault and showUsageStatus props of StorageSelector #2180

Conversation

ironAiken2 commented Jan 26, 2024 • edited by yomybaby Loading

This PR resolves #2151

aws-amplify-ap-northeast-2 bot commented Jan 26, 2024

This comment was marked as resolved.

lizable left a comment

Choose a reason for hiding this comment

This comment was marked as resolved.

This comment was marked as resolved.

lizable left a comment • edited Loading

Choose a reason for hiding this comment

lizable left a comment

Choose a reason for hiding this comment

github-actions bot commented Feb 1, 2024 • edited Loading

Coverage report for ./react

Test suite run success

ironAiken2 commented Feb 1, 2024

lizable left a comment

Choose a reason for hiding this comment

yomybaby Feb 6, 2024

Choose a reason for hiding this comment

yomybaby Feb 6, 2024

Choose a reason for hiding this comment

yomybaby Feb 8, 2024

Choose a reason for hiding this comment

yomybaby left a comment

Choose a reason for hiding this comment

yomybaby commented Feb 8, 2024

ironAiken2 commented Jan 26, 2024 •

edited by yomybaby

Loading

lizable left a comment •

edited

Loading

github-actions bot commented Feb 1, 2024 •

edited

Loading

Coverage report for `./react`