Skip to content

Commit 57f3390

Browse files
raintygaoopensearch-changeset-bot[bot]SuZhou-JoeHailong-am
authored
fix user appearance not working (opensearch-project#9700)
* update user level settings Signed-off-by: tygao <tygao@amazon.com> * Changeset file for PR opensearch-project#9700 created/updated * use ts-expect-error Signed-off-by: tygao <tygao@amazon.com> * remove extra check Signed-off-by: tygao <tygao@amazon.com> * update comments Signed-off-by: tygao <tygao@amazon.com> --------- Signed-off-by: tygao <tygao@amazon.com> Co-authored-by: opensearch-changeset-bot[bot] <154024398+opensearch-changeset-bot[bot]@users.noreply.github.com> Co-authored-by: SuZhou-Joe <suzhou@amazon.com> Co-authored-by: Hailong Cui <ihailong@amazon.com>
1 parent 412e11b commit 57f3390

File tree

4 files changed

+163
-45
lines changed

4 files changed

+163
-45
lines changed

‎changelogs/fragments/9700.yml‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,2 @@
1+
fix:
2+
- Fix user appearance not working ([#9700](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/9700))

‎src/core/public/ui_settings/ui_settings_client.test.ts‎

Lines changed: 101 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -448,3 +448,104 @@ describe('#overrideLocalDefault', () => {
448448
});
449449
});
450450
});
451+
452+
describe('preferBrowserSetting merge', () => {
453+
const originalLocalStorage = window.localStorage;
454+
455+
beforeEach(() => {
456+
// @ts-expect-error
457+
window.localStorage = {
458+
store: {},
459+
getItem(key: string) {
460+
return this.store[key] || null;
461+
},
462+
setItem(key: string, value: string) {
463+
this.store[key] = value;
464+
},
465+
removeItem(key: string) {
466+
delete this.store[key];
467+
},
468+
clear() {
469+
this.store = {};
470+
},
471+
};
472+
});
473+
474+
afterEach(() => {
475+
window.localStorage = originalLocalStorage;
476+
});
477+
478+
it('should use browser value if preferBrowserSetting=true', () => {
479+
window.localStorage.setItem(
480+
'uiSettings',
481+
JSON.stringify({
482+
'theme:darkMode': { userValue: true },
483+
})
484+
);
485+
const { client } = setup({
486+
defaults: {
487+
'theme:darkMode': { value: false, preferBrowserSetting: true },
488+
'theme:enableUserControl': { value: true },
489+
},
490+
initialSettings: {
491+
'theme:darkMode': { userValue: false, preferBrowserSetting: true },
492+
'theme:enableUserControl': { userValue: true },
493+
},
494+
});
495+
expect(client.get('theme:darkMode')).toBe(true);
496+
});
497+
498+
it('should use cache value if preferBrowserSetting=false', () => {
499+
window.localStorage.setItem(
500+
'uiSettings',
501+
JSON.stringify({
502+
'theme:darkMode': { userValue: true },
503+
})
504+
);
505+
const { client } = setup({
506+
defaults: {
507+
'theme:darkMode': { value: false, preferBrowserSetting: false },
508+
'theme:enableUserControl': { value: true },
509+
},
510+
initialSettings: {
511+
'theme:darkMode': { userValue: false, preferBrowserSetting: false },
512+
'theme:enableUserControl': { userValue: true },
513+
},
514+
});
515+
expect(client.get('theme:darkMode')).toBe(false);
516+
});
517+
518+
it('should fallback to cache if browser has no value', () => {
519+
window.localStorage.setItem('uiSettings', JSON.stringify({}));
520+
const { client } = setup({
521+
defaults: {
522+
'theme:darkMode': { value: false, preferBrowserSetting: true },
523+
'theme:enableUserControl': { value: true },
524+
},
525+
initialSettings: {
526+
'theme:darkMode': { userValue: true, preferBrowserSetting: true },
527+
'theme:enableUserControl': { userValue: true },
528+
},
529+
});
530+
expect(client.get('theme:darkMode')).toBe(true);
531+
});
532+
533+
it('should fallback to browser if cache has no value, preferBrowserSetting=true', () => {
534+
window.localStorage.setItem(
535+
'uiSettings',
536+
JSON.stringify({
537+
'theme:darkMode': { userValue: true },
538+
})
539+
);
540+
const { client } = setup({
541+
defaults: {
542+
'theme:darkMode': { value: false, preferBrowserSetting: true },
543+
'theme:enableUserControl': { value: true },
544+
},
545+
initialSettings: {
546+
'theme:enableUserControl': { userValue: true },
547+
},
548+
});
549+
expect(client.get('theme:darkMode')).toBe(true);
550+
});
551+
});

‎src/core/public/ui_settings/ui_settings_client.ts‎

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -62,7 +62,15 @@ export class UiSettingsClient implements IUiSettingsClient {
6262
this.cache['theme:enableUserControl']?.userValue ??
6363
this.cache['theme:enableUserControl']?.value
6464
) {
65-
this.cache = defaultsDeep(this.cache, this.getBrowserStoredSettings());
65+
const browserSettingsKeys = Object.keys(this.getBrowserStoredSettings());
66+
for (const key of browserSettingsKeys) {
67+
// Only keep settings which is declared as preferBrowserSetting as high priority instead of all settings, otherwise all settings could be replaced value easily if they are declared in localStorage.
68+
const preferBrowser = this.cache[key]?.preferBrowserSetting ?? false;
69+
if (preferBrowser) {
70+
// browser level setting should have a higher priority, otherwise its userValue could not be consumed.
71+
this.cache[key] = defaultsDeep({}, this.getBrowserStoredSettings()[key], this.cache[key]);
72+
}
73+
}
6674
}
6775

6876
params.done$.subscribe({

‎src/plugins/advanced_settings/public/header_user_theme_menu.tsx‎

Lines changed: 51 additions & 44 deletions
Original file line numberDiff line numberDiff line change
@@ -28,11 +28,11 @@ export const HeaderUserThemeMenu = () => {
2828
const {
2929
services: { uiSettings },
3030
} = useOpenSearchDashboards<CoreStart>();
31-
const themeOptions = Object.keys(themeVersionLabelMap).map((v) => ({
31+
const themeVersionOptions = Object.keys(themeVersionLabelMap).map((v) => ({
3232
value: v,
3333
text: themeVersionLabelMap[v],
3434
}));
35-
const screenModeOptions = [
35+
const colorModeOptions = [
3636
{
3737
value: 'light',
3838
text: 'Light mode',
@@ -46,63 +46,66 @@ export const HeaderUserThemeMenu = () => {
4646
text: 'Use browser settings',
4747
},
4848
];
49-
const defaultTheme = uiSettings.getDefault('theme:version');
50-
const defaultScreenMode = uiSettings.getDefault('theme:darkMode');
51-
const prefersAutomatic =
49+
const defaultThemeVersion = uiSettings.getDefault('theme:version');
50+
const defaultIsDarkMode = uiSettings.getDefault('theme:darkMode');
51+
const isUsingBrowserColorScheme =
5252
(window.localStorage.getItem('useBrowserColorScheme') && window.matchMedia) || false;
53-
const [darkMode, setDarkMode] = useUiSetting$<boolean>('theme:darkMode');
53+
const [isDarkMode, setIsDarkMode] = useUiSetting$<boolean>('theme:darkMode');
5454
const [themeVersion, setThemeVersion] = useUiSetting$<string>('theme:version');
55-
const [isPopoverOpen, setPopover] = useState(false);
56-
// TODO: improve naming?
57-
const [theme, setTheme] = useState(
58-
themeOptions.find((t) => t.value === themeVersionValueMap[themeVersion])?.value ||
59-
themeVersionValueMap[defaultTheme]
55+
const [isPopoverOpen, setIsPopoverOpen] = useState(false);
56+
57+
const [selectedThemeVersion, setSelectedThemeVersion] = useState(
58+
themeVersionOptions.find((t) => t.value === themeVersionValueMap[themeVersion])?.value ||
59+
themeVersionValueMap[defaultThemeVersion]
6060
);
61-
const [screenMode, setScreenMode] = useState(
62-
prefersAutomatic
63-
? screenModeOptions[2].value
64-
: darkMode
65-
? screenModeOptions[1].value
66-
: screenModeOptions[0].value
61+
const [selectedColorMode, setSelectedColorMode] = useState(
62+
isUsingBrowserColorScheme
63+
? colorModeOptions[2].value
64+
: isDarkMode
65+
? colorModeOptions[1].value
66+
: colorModeOptions[0].value
6767
);
68-
6968
const useLegacyAppearance = !uiSettings.get('home:useNewHomePage');
7069

71-
const onButtonClick = () => {
72-
setPopover(!isPopoverOpen);
70+
const togglePopover = () => {
71+
setIsPopoverOpen(!isPopoverOpen);
7372
};
7473

75-
const onThemeChange = (e: ChangeEvent<HTMLSelectElement>) => {
76-
setTheme(e.target.value);
74+
const handleThemeVersionChange = (e: ChangeEvent<HTMLSelectElement>) => {
75+
setSelectedThemeVersion(e.target.value);
7776
};
7877

79-
const onScreenModeChange = (e: ChangeEvent<HTMLSelectElement>) => {
80-
setScreenMode(e.target.value);
78+
const handleColorModeChange = (e: ChangeEvent<HTMLSelectElement>) => {
79+
setSelectedColorMode(e.target.value);
8180
};
8281

83-
const onAppearanceSubmit = async (e: SyntheticEvent) => {
84-
const actions = [setThemeVersion(themeOptions.find((t) => theme === t.value)?.value ?? '')];
82+
const applyAppearanceSettings = async (e: SyntheticEvent) => {
83+
const pendingActions = [
84+
setThemeVersion(
85+
themeVersionOptions.find((t) => selectedThemeVersion === t.value)?.value ?? ''
86+
),
87+
];
8588

86-
if (screenMode === 'automatic') {
87-
const browserMode = window.matchMedia('(prefers-color-scheme: dark)').matches;
89+
if (selectedColorMode === 'automatic') {
90+
const systemPrefersDarkMode = window.matchMedia('(prefers-color-scheme: dark)').matches;
8891
window.localStorage.setItem('useBrowserColorScheme', 'true');
8992

90-
if (browserMode !== darkMode) {
91-
actions.push(setDarkMode(browserMode));
93+
if (systemPrefersDarkMode !== isDarkMode) {
94+
pendingActions.push(setIsDarkMode(systemPrefersDarkMode));
9295
}
93-
} else if ((screenMode === 'dark') !== darkMode) {
94-
actions.push(setDarkMode(screenMode === 'dark'));
96+
} else if ((selectedColorMode === 'dark') !== isDarkMode) {
97+
pendingActions.push(setIsDarkMode(selectedColorMode === 'dark'));
9598
window.localStorage.removeItem('useBrowserColorScheme');
9699
} else {
97100
window.localStorage.removeItem('useBrowserColorScheme');
98101
}
99102
// TODO: only set changed
100-
await await Promise.all([actions]);
103+
await Promise.all(pendingActions);
101104
window.location.reload();
102105
};
103106

104107
const closePopover = () => {
105-
setPopover(false);
108+
setIsPopoverOpen(false);
106109
};
107110

108111
const innerButton = useLegacyAppearance ? (
@@ -112,7 +115,7 @@ export const HeaderUserThemeMenu = () => {
112115
aria-label={i18n.translate('advancedSettings.headerGlobalNav.themeMenuButtonAriaLabel', {
113116
defaultMessage: 'Appearance menu',
114117
})}
115-
onClick={onButtonClick}
118+
onClick={togglePopover}
116119
>
117120
<EuiIcon type="color" size="m" />
118121
</EuiHeaderSectionItemButton>
@@ -126,7 +129,7 @@ export const HeaderUserThemeMenu = () => {
126129
aria-label={i18n.translate('advancedSettings.headerGlobalNav.themeMenuButtonAriaLabel', {
127130
defaultMessage: 'Appearance menu',
128131
})}
129-
onClick={onButtonClick}
132+
onClick={togglePopover}
130133
/>
131134
);
132135

@@ -146,22 +149,26 @@ export const HeaderUserThemeMenu = () => {
146149
// TODO: fix focus behavior
147150
const appearanceContent = (
148151
<div style={{ maxWidth: 300 }}>
149-
<EuiCompressedFormRow label="Theme version" helpText={`Default: ${defaultTheme}`}>
150-
<EuiCompressedSelect options={themeOptions} value={theme} onChange={onThemeChange} />
152+
<EuiCompressedFormRow label="Theme version" helpText={`Default: ${defaultThemeVersion}`}>
153+
<EuiCompressedSelect
154+
options={themeVersionOptions}
155+
value={selectedThemeVersion}
156+
onChange={handleThemeVersionChange}
157+
/>
151158
</EuiCompressedFormRow>
152159
<EuiCompressedFormRow
153160
label="Screen mode"
154161
helpText={`Default: ${
155-
screenModeOptions.find((t) => {
156-
const defaultValue = defaultScreenMode ? 'dark' : 'light';
162+
colorModeOptions.find((t) => {
163+
const defaultValue = defaultIsDarkMode ? 'dark' : 'light';
157164
return defaultValue === t.value;
158165
})?.text
159166
}`}
160167
>
161168
<EuiCompressedSelect
162-
options={screenModeOptions}
163-
value={screenMode}
164-
onChange={onScreenModeChange}
169+
options={colorModeOptions}
170+
value={selectedColorMode}
171+
onChange={handleColorModeChange}
165172
/>
166173
</EuiCompressedFormRow>
167174
<EuiFlexGroup>
@@ -178,7 +185,7 @@ export const HeaderUserThemeMenu = () => {
178185
<EuiFlexItem grow={false}>
179186
<EuiCompressedFormRow hasEmptyLabelSpace>
180187
{/* TODO: disable submit until changes */}
181-
<EuiSmallButton onClick={onAppearanceSubmit} type="submit">
188+
<EuiSmallButton onClick={applyAppearanceSettings} type="submit">
182189
Apply
183190
</EuiSmallButton>
184191
</EuiCompressedFormRow>

0 commit comments

Comments
 (0)
⚡