Skip to content

Commit 6462708

Browse files
committed
refactor(FR-1720): update setting components to use onChange instead of setValue for better clarity (#4849)
Resolves #4689 ([FR-1720](https://lablup.atlassian.net/browse/FR-1720)) ## Refactor SettingItem component to use strongly typed props and improve onChange handling This PR refactors the `SettingItem` component to use strongly typed props with TypeScript discriminated unions, making the component more type-safe. It also standardizes the API by replacing `setValue` with `onChange` across all usages. Key changes: - Replace generic `setValue` prop with a more consistent `onChange` callback - Create type-specific props interfaces for each setting type (checkbox, select, custom) - Add proper type annotations to prevent type errors - Simplify reset logic and dropdown visibility conditions - Remove unnecessary `onAfterChange` in favor of a single callback pattern [FR-1720]: https://lablup.atlassian.net/browse/FR-1720?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ
1 parent 0695326 commit 6462708

4 files changed

Lines changed: 84 additions & 60 deletions

File tree

react/src/components/ConfigurationsSettingList.tsx

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -159,7 +159,8 @@ const ConfigurationsSettingList = () => {
159159
},
160160
defaultValue: defaultConfigurationsSettings.image_pulling_behavior,
161161
value: options.image_pulling_behavior,
162-
setValue: (value) => setImagePullingBehavior(value),
162+
onChange: (value) =>
163+
setImagePullingBehavior(value as ImagePullingBehavior),
163164
},
164165
{
165166
type: 'custom',

react/src/components/SettingItem.tsx

Lines changed: 70 additions & 46 deletions
Original file line numberDiff line numberDiff line change
@@ -16,21 +16,46 @@ import { t } from 'i18next';
1616
import _ from 'lodash';
1717
import React, { ReactElement, ReactNode, useState } from 'react';
1818

19-
export interface SettingItemProps {
19+
type BaseSettingItemProps = {
2020
'data-testid'?: string;
21-
type: 'custom' | 'checkbox' | 'select';
2221
title: string;
2322
description?: string | ReactElement;
2423
children?: ReactNode;
25-
defaultValue?: any;
26-
value?: any;
27-
setValue?: (value: any) => void;
2824
showResetButton?: boolean;
25+
onReset?: () => void;
26+
};
27+
28+
type CheckboxSettingItemProps = BaseSettingItemProps & {
29+
type: 'checkbox';
30+
defaultValue?: boolean;
31+
value?: boolean;
32+
onChange?: (value?: boolean) => void;
2933
checkboxProps?: Omit<CheckboxProps, 'value' | 'onChange' | 'defaultValue'>;
34+
selectProps?: never;
35+
};
36+
37+
type SelectSettingItemProps = BaseSettingItemProps & {
38+
type: 'select';
39+
defaultValue?: string | number;
40+
value?: string | number;
41+
onChange?: (value?: string | number) => void;
3042
selectProps?: Omit<BAISelectProps, 'value' | 'onChange' | 'defaultValue'>;
31-
onAfterChange?: (value: any) => void;
32-
onReset?: () => void;
33-
}
43+
checkboxProps?: never;
44+
};
45+
46+
type CustomSettingItemProps = BaseSettingItemProps & {
47+
type: 'custom';
48+
defaultValue?: any;
49+
value?: any;
50+
onChange?: (value?: any) => void;
51+
selectProps?: never;
52+
checkboxProps?: never;
53+
};
54+
55+
export type SettingItemProps =
56+
| CheckboxSettingItemProps
57+
| SelectSettingItemProps
58+
| CustomSettingItemProps;
3459

3560
const useStyles = createStyles(({ css }) => ({
3661
baiSettingItemCheckbox: css`
@@ -49,8 +74,7 @@ const SettingItem: React.FC<SettingItemProps> = ({
4974
children,
5075
defaultValue,
5176
value,
52-
setValue,
53-
onAfterChange,
77+
onChange,
5478
onReset,
5579
selectProps,
5680
checkboxProps,
@@ -67,13 +91,16 @@ const SettingItem: React.FC<SettingItemProps> = ({
6791
const resetItem = () => {
6892
if (onReset) {
6993
onReset();
70-
} else {
71-
!selectProps?.disabled &&
72-
!checkboxProps?.disabled &&
73-
setValue?.(defaultValue);
94+
} else if (isEnabled && onChange) {
95+
onChange(defaultValue);
7496
}
7597
};
7698

99+
const isEnabled =
100+
(type === 'select' && !selectProps?.disabled) ||
101+
(type === 'checkbox' && !checkboxProps?.disabled) ||
102+
type === 'custom';
103+
77104
return (
78105
<BAIFlex
79106
data-testid={dataTestId}
@@ -93,38 +120,37 @@ const SettingItem: React.FC<SettingItemProps> = ({
93120
>
94121
{title}
95122
</Typography.Text>
96-
{(!selectProps?.disabled || !checkboxProps?.disabled) &&
123+
{isEnabled &&
97124
value !== undefined &&
98125
value !== null &&
99126
defaultValue !== value && <Badge dot status="warning" />}
100-
{!selectProps?.disabled &&
101-
!checkboxProps?.disabled &&
102-
showResetButton && (
103-
<Dropdown
104-
menu={{
105-
items: [
106-
{
107-
key: 'reset',
108-
label: t('button.Reset'),
109-
onClick: () => setIsOpenResetChangesModal(),
110-
danger: true,
111-
},
112-
],
127+
{isEnabled && showResetButton && (
128+
<Dropdown
129+
menu={{
130+
items: [
131+
{
132+
key: 'reset',
133+
label: t('button.Reset'),
134+
onClick: () => setIsOpenResetChangesModal(),
135+
danger: true,
136+
},
137+
],
138+
}}
139+
placement="topLeft"
140+
onOpenChange={(e) => e && setIsDropdownOpen(true)}
141+
>
142+
<BAIButton
143+
icon={<SettingOutlined />}
144+
type="text"
145+
style={{
146+
width: 20,
147+
height: 20,
148+
opacity: isDropdownOpen ? 1 : 0,
149+
transition: 'opacity 0.2s ease-in-out',
113150
}}
114-
onOpenChange={(e) => e && setIsDropdownOpen(true)}
115-
>
116-
<BAIButton
117-
icon={<SettingOutlined />}
118-
type="text"
119-
style={{
120-
width: 20,
121-
height: 20,
122-
opacity: isDropdownOpen ? 1 : 0,
123-
transition: 'opacity 0.2s ease-in-out',
124-
}}
125-
/>
126-
</Dropdown>
127-
)}
151+
/>
152+
</Dropdown>
153+
)}
128154
</BAIFlex>
129155
</BAIFlex>
130156
{type === 'custom' && (
@@ -143,8 +169,7 @@ const SettingItem: React.FC<SettingItemProps> = ({
143169
className={styles.baiSettingItemCheckbox}
144170
checked={value}
145171
onChange={(e) => {
146-
setValue?.(e.target.checked);
147-
onAfterChange?.(e);
172+
onChange?.(e.target.checked);
148173
}}
149174
{...checkboxProps}
150175
>
@@ -166,8 +191,7 @@ const SettingItem: React.FC<SettingItemProps> = ({
166191
value={value}
167192
popupMatchSelectWidth={false}
168193
onChange={(value) => {
169-
setValue?.(value);
170-
onAfterChange?.(value);
194+
onChange?.(value);
171195
}}
172196
style={{
173197
marginTop: token.marginXS,

react/src/components/SettingList.tsx

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -307,8 +307,7 @@ const resetSettingItems = (settingGroups: SettingGroup[]) => {
307307
} else {
308308
!option?.selectProps?.disabled &&
309309
!option?.checkboxProps?.disabled &&
310-
option?.setValue &&
311-
option.setValue(option.defaultValue);
310+
option?.onChange?.(option.defaultValue);
312311
}
313312
});
314313
};

react/src/pages/UserSettingsPage.tsx

Lines changed: 11 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -103,11 +103,11 @@ const UserPreferencesPage = () => {
103103
description: <Trans i18nKey="userSettings.DescDesktopNotification" />,
104104
defaultValue: false,
105105
value: desktopNotification,
106-
setValue: setDesktopNotification,
107-
onAfterChange: (e: any) => {
106+
onChange: (value) => {
107+
setDesktopNotification(value);
108+
108109
// Request permission for desktop notifications
109-
if (!e.target.checked || Notification.permission === 'granted')
110-
return;
110+
if (!value || Notification.permission === 'granted') return;
111111
if (!('Notification' in window)) {
112112
message.error(t('desktopNotification.NotSupported'));
113113
setDesktopNotification(false);
@@ -133,7 +133,7 @@ const UserPreferencesPage = () => {
133133
description: <Trans i18nKey="userSettings.DescUseCompactSidebar" />,
134134
defaultValue: false,
135135
value: compactSidebar,
136-
setValue: setCompactSidebar,
136+
onChange: setCompactSidebar,
137137
},
138138
{
139139
'data-testid': 'items-language-select',
@@ -165,7 +165,7 @@ const UserPreferencesPage = () => {
165165
},
166166
defaultValue: defaultLanguage,
167167
value: selectedLanguage || defaultLanguage,
168-
setValue: (value: any) => {
168+
onChange: (value: any) => {
169169
setSelectedLanguage(value);
170170
setLanguage(value);
171171
const event = new CustomEvent('language-changed', {
@@ -185,7 +185,7 @@ const UserPreferencesPage = () => {
185185
),
186186
defaultValue: false,
187187
value: preserveLogin,
188-
setValue: setPreserveLogin,
188+
onChange: setPreserveLogin,
189189
},
190190
{
191191
'data-testid': 'items-automatic-update-check',
@@ -196,7 +196,7 @@ const UserPreferencesPage = () => {
196196
),
197197
defaultValue: false,
198198
value: autoAutomaticUpdateCheck,
199-
setValue: setAutoAutomaticUpdateCheck,
199+
onChange: setAutoAutomaticUpdateCheck,
200200
},
201201
{
202202
'data-testid': 'items-auto-logout',
@@ -205,7 +205,7 @@ const UserPreferencesPage = () => {
205205
description: t('userSettings.DescAutoLogout'),
206206
defaultValue: false,
207207
value: autoLogout,
208-
setValue: setAutoLogout,
208+
onChange: setAutoLogout,
209209
},
210210
{
211211
'data-testid': 'items-my-keypair-info',
@@ -264,7 +264,7 @@ const UserPreferencesPage = () => {
264264
},
265265
defaultValue: 2,
266266
value: maxConcurrentUpload || 2,
267-
setValue: setMaxConcurrentUpload,
267+
onChange: (value) => setMaxConcurrentUpload(_.toNumber(value)),
268268
},
269269
]),
270270
},
@@ -320,7 +320,7 @@ const UserPreferencesPage = () => {
320320
description: t('general.Enabled'),
321321
defaultValue: false,
322322
value: experimentalAIAgents,
323-
setValue: setExperimentalAIAgents,
323+
onChange: setExperimentalAIAgents,
324324
},
325325
],
326326
},

0 commit comments

Comments
 (0)