refactor(FR-357): NEO configurations page using Setting List #3077

nowgnuesLee · 2025-01-26T09:31:13Z

resolves #3015 (FR-357)

changes

Changed configurations items into react SettingList component
Update modal design to current design system. Enable to auto-save and delete fuctionality.
Changed notification to message
add Save button and validate.
add note component on upside.

Checklist: (if applicable)

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

graphite-app · 2025-01-26T09:31:18Z

How to use the Graphite Merge Queue

Add either label to this PR to merge it via the merge queue:

flow:merge-queue - adds this PR to the back of the merge queue
flow:hotfix - for urgent hot fixes, skip the queue and merge this PR next

You must have a Graphite account in order to use the merge queue. Sign up using this link.

_{An organization admin has required the Graphite Merge Queue in this repository.} _{Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue.}

github-actions · 2025-01-26T09:32:38Z

Coverage report for `./react`

Action wasn't able to generate report within GitHub comment limit. If you're facing this issue, please let me know by commenting under this issue.

Report generated by 🧪jest coverage report action from 5379e43

…o refactor/configurations-react

agatha197

Please resolve the conflicts. And the margin between the text and the button is too narrow. Let's enlarge it.

…o refactor/configurations-react

lizable

I have a several requests to change before this PR is merged.

Update on-the-fly
I'm not sure update on blur event is a good choice or not.
In both "overlay network" setting and "scheduler". I think is better to have "update" button to click so that they know what they've been changed, explicitly. Besides, everytime input field is on blur, update happens; which makes continuous update requests with snackbar, which seems a bit distracted. Therefore I guess It's better to have "update" button in the modal for user to request explicitly. If you have any other options, please tell us.
Screen Recording 2025-02-13 at 5.44.59 PM.mov (uploaded via Graphite)
Would Snackbar notification is the best option?
I'm not sure about this either...probably after updating the request from above will make it okay, but I think we should make the height of notification(snack bar) little bit shortened than current status. I think is too big to distract the changes.

nowgnuesLee · 2025-02-13T09:36:01Z

I think so. Previously, a warning modal would appear when user doesn't save the value and close it, but I removed that modal and changed it to save on-the-fly using onBlur (related thread. What do you think about displaying a warning message above instead of the warning modal? I think it would be better. After that let's add update button.

I used that notification due to the previous legacy. How about using the message feature from App.useApp() instead?

…o refactor/configurations-react

lizable

@nowgnuesLee It would be nice to fix the conflict so that I can review this :)

yomybaby · 2025-02-21T05:05:35Z

react/src/App.tsx

@@ -354,7 +357,11 @@ const router = createBrowserRouter([
      },
      {
        path: '/settings',
-        handle: { labelKey: 'webui.menu.Configurations' },


Please recover handle. It's for display breadcrumb title.

Oh, I'm sorry. I'll recover it.

yomybaby · 2025-02-21T05:14:34Z

react/src/hooks/useBAIConfigurationsSetting.ts

+      );
+      if (result === 'ok') {
+        updatePulling();
+        message.success(t('notification.SuccessfullyUpdated'));


It's not a good idea to call a function to render the UI (e.g., message.success). My suggestion is to remove useBAIConfigurationSettings and move all the logic from this file to each item in ConfigurationSettingList.tsx. Since the logic for each item is neither lengthy nor dependent, it would be better to keep the related code close together to enhance readability.

I had been thinking about it as well, but your suggestion has made it clearer. Thank you.

yomybaby · 2025-02-21T05:17:20Z

react/src/components/SettingItem.tsx

+              ...selectProps?.style,
+              marginTop: token.marginXS,


The style from the prop should have higher priority. More specifically defined styles should take precedence.

Thank you! I'll fix it up.

yomybaby · 2025-02-21T05:24:06Z

react/src/components/OverlayNetworkSettingModal.tsx

+              <Flex>
+                {labels[key]}
+                <Tooltip title={tooltipText[key]}>
+                  <Button type="link" icon={<InfoCircleOutlined />} />


Form.Item has 'tooltip` prop. Please use it. :)

Oh, I didn't know about it. Thank you for pointing that out. :)

yomybaby · 2025-02-21T05:26:50Z

react/src/components/OverlayNetworkSettingModal.tsx

+          <Form.Item
+            label={
+              <Flex>
+                {labels[key]}
+                <Tooltip title={tooltipText[key]}>
+                  <Button type="link" icon={<InfoCircleOutlined />} />
+                </Tooltip>
+              </Flex>
+            }
+            required
+            rules={[
+              {
+                required: true,
+                message: t('data.explorer.ValueRequired'),
+              },
+            ]}
+            name={key}
+            key={key}
+          >
+            <InputNumber
+              min={optionRange[key].min}
+              max={optionRange[key].max}
+              style={{
+                width: '100%',
+              }}
+            />
+          </Form.Item>


It doesn't seem necessary to manage networkOptions as a separate array and use the map function to create the UI. It looks like you did this considering the possibility of adding more options in the future. However, currently, there's only one option, and there's no guarantee that future additions will follow the same rule or use InputNumber.

I agree. I will modify everything that was managed as an array. Thank you! :)

…o refactor/configurations-react

refactor: change configurations items into settinglist items

7ac823d

github-actions bot assigned nowgnuesLee Jan 26, 2025

github-actions bot added the size:L 100~500 LoC label Jan 26, 2025

nowgnuesLee added 6 commits January 31, 2025 10:38

feature: adding configuration page

ce4738f

feature: add configurations settings state

e25177d

feat: implement overlay network feature

5987b73

feat: implement feature for chainging scheduler type

84db5fd

refactor: changed configurations page into react component

8846677

Merge branch 'main' of https://github.com/lablup/backend.ai-webui int…

5e59f60

…o refactor/configurations-react

github-actions bot added area:ux UI / UX issue. area:i18n Localization size:XL 500~ LoC and removed size:L 100~500 LoC labels Feb 4, 2025

fix: code to comply with eslint rules

0857ae5

nowgnuesLee marked this pull request as ready for review February 4, 2025 07:07

nowgnuesLee requested review from yomybaby, agatha197 and lizable February 4, 2025 07:07

nowgnuesLee marked this pull request as draft February 5, 2025 06:57

feat: add tooltip to modals in configurations page

497eb06

nowgnuesLee marked this pull request as ready for review February 5, 2025 07:54

agatha197 requested changes Feb 10, 2025

View reviewed changes

nowgnuesLee added 3 commits February 10, 2025 18:00

Merge branch 'main' of https://github.com/lablup/backend.ai-webui int…

8e301b0

…o refactor/configurations-react

feature: add margin to SettingItem's items

8b7b4b6

fix: remove margin from SettingItem's Checkbox

762f8b2

nowgnuesLee requested a review from agatha197 February 10, 2025 09:35

lizable requested changes Feb 13, 2025

View reviewed changes

nowgnuesLee added 3 commits February 17, 2025 14:43

feat: apply message, update button, and validate

6bdd9c5

fix: revised for proper grammar

459da94

Merge branch 'main' of https://github.com/lablup/backend.ai-webui int…

d63b056

…o refactor/configurations-react

nowgnuesLee requested a review from lizable February 17, 2025 05:51

nowgnuesLee added 2 commits February 18, 2025 10:39

Merge branch 'main' into refactor/configurations-react

a091760

feat: update design

33a24e5

lizable requested changes Feb 21, 2025

View reviewed changes

yomybaby requested changes Feb 21, 2025

View reviewed changes

nowgnuesLee added 5 commits February 21, 2025 19:28

fix: recover handler in App.tsx

97c04d6

fix: fix the width of form item

1e4308e

Merge branch 'main' of https://github.com/lablup/backend.ai-webui int…

ca30f22

…o refactor/configurations-react

chore: update i18n

4f8115a

refactor: move config features to configurationSettingList

5379e43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(FR-357): NEO configurations page using Setting List #3077

refactor(FR-357): NEO configurations page using Setting List #3077

nowgnuesLee commented Jan 26, 2025 •

edited

Loading

graphite-app bot commented Jan 26, 2025

github-actions bot commented Jan 26, 2025 •

edited

Loading

agatha197 left a comment

lizable left a comment •

edited

Loading

nowgnuesLee commented Feb 13, 2025

lizable left a comment

yomybaby Feb 21, 2025

nowgnuesLee Feb 21, 2025

yomybaby Feb 21, 2025

nowgnuesLee Feb 21, 2025

yomybaby Feb 21, 2025

nowgnuesLee Feb 21, 2025

yomybaby Feb 21, 2025

nowgnuesLee Feb 21, 2025

yomybaby Feb 21, 2025

nowgnuesLee Feb 21, 2025

refactor(FR-357): NEO configurations page using Setting List #3077

Are you sure you want to change the base?

refactor(FR-357): NEO configurations page using Setting List #3077

Conversation

nowgnuesLee commented Jan 26, 2025 • edited Loading

graphite-app bot commented Jan 26, 2025

How to use the Graphite Merge Queue

github-actions bot commented Jan 26, 2025 • edited Loading

Coverage report for ./react

agatha197 left a comment

Choose a reason for hiding this comment

lizable left a comment • edited Loading

Choose a reason for hiding this comment

nowgnuesLee commented Feb 13, 2025

lizable left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nowgnuesLee commented Jan 26, 2025 •

edited

Loading

github-actions bot commented Jan 26, 2025 •

edited

Loading

Coverage report for `./react`

lizable left a comment •

edited

Loading