Skip to content

Commit 9d67acb

Browse files
rahimrahmanrahimrahman
authored
fix(MM-63596): custom theme cannot be reselected (#8960)
* initialTheme should not be a dependent of useCallback since its a ref * a few cleanup on test to make it more clearer * add a bit more description * fix(MM-64710): changing themes repeatedly in rapid succession cause the theme to not register (#8980) * add more test, doubleTap prevention test * changes based on comments * remove the use of currentTheme * consolidate handleSelectTheme with setThemePreference * add comment to explain why we're storing customTheme in a state
1 parent 6678003 commit 9d67acb

File tree

2 files changed

+308
-25
lines changed

2 files changed

+308
-25
lines changed

‎app/screens/settings/display_theme/display_theme.test.tsx‎

Lines changed: 290 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,290 @@
1+
// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved.
2+
// See LICENSE.txt for license information.
3+
4+
import {fireEvent, render, screen, waitFor} from '@testing-library/react-native';
5+
import React from 'react';
6+
import {BackHandler} from 'react-native';
7+
8+
import {savePreference} from '@actions/remote/preference';
9+
import {Preferences} from '@constants';
10+
import {useTheme} from '@context/theme';
11+
import {popTopScreen} from '@screens/navigation';
12+
import NavigationStore from '@store/navigation_store';
13+
import {renderWithIntl} from '@test/intl-test-helper';
14+
15+
import DisplayTheme from './display_theme';
16+
17+
import type {AvailableScreens} from '@typings/screens/navigation';
18+
19+
jest.mock('@screens/navigation');
20+
jest.mock('@context/theme', () => ({
21+
useTheme: jest.fn(),
22+
}));
23+
jest.mock('@actions/remote/preference');
24+
jest.mock('@store/navigation_store');
25+
26+
const displayThemeOtherProps = {
27+
componentId: 'DisplayTheme' as AvailableScreens,
28+
currentTeamId: '1',
29+
currentUserId: '1',
30+
};
31+
32+
describe('DisplayTheme', () => {
33+
34+
beforeEach(() => {
35+
jest.clearAllMocks();
36+
jest.mocked(useTheme).mockImplementation(() => ({...Preferences.THEMES.denim, type: 'Denim'}));
37+
});
38+
39+
it('should render with a few themes, denim selected', () => {
40+
render(
41+
<DisplayTheme
42+
allowedThemeKeys={['denim', 'sapphire']}
43+
{...displayThemeOtherProps}
44+
/>);
45+
46+
expect(screen.getByTestId('theme_display_settings.denim.option')).toBeTruthy();
47+
expect(screen.getByTestId('theme_display_settings.denim.option.selected')).toBeTruthy();
48+
49+
expect(screen.getByTestId('theme_display_settings.sapphire.option')).toBeTruthy();
50+
expect(screen.queryByTestId('theme_display_settings.sapphire.option.selected')).toBeFalsy();
51+
});
52+
53+
it('should render with custom theme, current theme is custom', () => {
54+
jest.mocked(useTheme).mockImplementation(() => ({...Preferences.THEMES.denim, type: 'custom'}));
55+
56+
renderWithIntl(
57+
<DisplayTheme
58+
allowedThemeKeys={['denim', 'custom']}
59+
{...displayThemeOtherProps}
60+
/>);
61+
62+
expect(screen.getByTestId('theme_display_settings.custom.option')).toBeTruthy();
63+
expect(screen.getByTestId('theme_display_settings.custom.option.selected')).toBeTruthy();
64+
});
65+
66+
it('should render with custom theme (default) and user change to denim (non-custom)', async () => {
67+
jest.mocked(useTheme).mockImplementation(() => ({...Preferences.THEMES.denim, type: 'custom'}));
68+
69+
renderWithIntl(
70+
<DisplayTheme
71+
allowedThemeKeys={['denim', 'custom']}
72+
{...displayThemeOtherProps}
73+
/>);
74+
75+
expect(screen.getByTestId('theme_display_settings.custom.option')).toBeTruthy();
76+
expect(screen.getByTestId('theme_display_settings.custom.option.selected')).toBeTruthy();
77+
78+
const denimTile = screen.getByTestId('theme_display_settings.denim.option');
79+
80+
fireEvent.press(denimTile);
81+
82+
await waitFor(() => {
83+
expect(savePreference).toHaveBeenCalledWith(
84+
expect.any(String),
85+
expect.arrayContaining([
86+
expect.objectContaining({
87+
category: 'theme',
88+
value: expect.stringContaining('"type":"Denim"'),
89+
}),
90+
]),
91+
);
92+
expect(savePreference).toHaveBeenCalledTimes(1);
93+
});
94+
95+
// since we're mocking useTheme and savePreference, savePreference will post changes to the backend API, and upon success,
96+
// it will update the `theme` preference via useTheme hook.
97+
jest.mocked(useTheme).mockImplementation(() => ({...Preferences.THEMES.denim, type: 'Denim'}));
98+
99+
// clearing the savePreference mock to show that it will not be called again after re-rendering the component
100+
jest.mocked(savePreference).mockClear();
101+
102+
screen.rerender(
103+
<DisplayTheme
104+
allowedThemeKeys={['denim', 'custom']}
105+
{...displayThemeOtherProps}
106+
/>,
107+
);
108+
109+
expect(savePreference).toHaveBeenCalledTimes(0);
110+
111+
expect(screen.getByTestId('theme_display_settings.denim.option.selected')).toBeTruthy();
112+
});
113+
114+
it('should render only with custom theme, it gets de-selected, and then user re-selects it', async () => {
115+
jest.mocked(useTheme).mockImplementation(() => ({...Preferences.THEMES.denim, type: 'custom'}));
116+
117+
renderWithIntl(
118+
<DisplayTheme
119+
allowedThemeKeys={[]}
120+
{...displayThemeOtherProps}
121+
/>);
122+
123+
jest.mocked(useTheme).mockImplementation(() => ({...Preferences.THEMES.denim, type: 'Denim'}));
124+
125+
screen.rerender(
126+
<DisplayTheme
127+
allowedThemeKeys={[]}
128+
{...displayThemeOtherProps}
129+
/>);
130+
131+
const customTile = screen.getByTestId('theme_display_settings.custom.option');
132+
133+
fireEvent.press(customTile);
134+
135+
await waitFor(() => {
136+
expect(savePreference).toHaveBeenCalledWith(
137+
expect.any(String),
138+
expect.arrayContaining([
139+
expect.objectContaining({
140+
category: 'theme',
141+
value: expect.stringContaining('"type":"custom"'),
142+
}),
143+
]),
144+
);
145+
});
146+
});
147+
148+
it('should render denim, then a different client set the theme to custom, and this client should render custom and automatically switch to it', () => {
149+
renderWithIntl(
150+
<DisplayTheme
151+
allowedThemeKeys={['denim']}
152+
{...displayThemeOtherProps}
153+
/>);
154+
155+
expect(screen.queryByTestId('theme_display_settings.custom.option.selected')).toBeFalsy();
156+
157+
jest.mocked(useTheme).mockImplementation(() => ({...Preferences.THEMES.denim, type: 'custom'}));
158+
159+
screen.rerender(
160+
<DisplayTheme
161+
allowedThemeKeys={['denim']}
162+
{...displayThemeOtherProps}
163+
/>);
164+
165+
expect(screen.getByTestId('theme_display_settings.custom.option.selected')).toBeTruthy();
166+
});
167+
168+
it('should not call popTopScreen (closes the screen) when changing theme', async () => {
169+
renderWithIntl(
170+
<DisplayTheme
171+
allowedThemeKeys={['denim', 'sapphire']}
172+
{...displayThemeOtherProps}
173+
/>,
174+
);
175+
176+
const sapphireTile = screen.getByTestId('theme_display_settings.sapphire.option');
177+
178+
fireEvent.press(sapphireTile);
179+
180+
jest.mocked(useTheme).mockImplementation(() => ({...Preferences.THEMES.sapphire, type: 'Sapphire'}));
181+
182+
screen.rerender(
183+
<DisplayTheme
184+
allowedThemeKeys={['denim', 'sapphire']}
185+
{...displayThemeOtherProps}
186+
/>,
187+
);
188+
189+
await waitFor(() => {
190+
expect(screen.getByTestId('theme_display_settings.sapphire.option.selected')).toBeTruthy();
191+
});
192+
193+
expect(popTopScreen).toHaveBeenCalledTimes(0);
194+
});
195+
196+
it('should call popTopScreen when Android back button is pressed', () => {
197+
(NavigationStore.getVisibleScreen as jest.Mock).mockReturnValue('DisplayTheme');
198+
const androidBackButtonHandler = jest.spyOn(BackHandler, 'addEventListener');
199+
200+
renderWithIntl(
201+
<DisplayTheme
202+
allowedThemeKeys={['denim', 'custom']}
203+
{...displayThemeOtherProps}
204+
/>,
205+
);
206+
207+
// simulate Android back button press
208+
androidBackButtonHandler.mock.calls[0][1]();
209+
210+
expect(popTopScreen).toHaveBeenCalledTimes(1);
211+
});
212+
213+
it('should allow user to select two different themes using normal interaction', async () => {
214+
jest.useFakeTimers();
215+
216+
const numOfSavePreferenceCalls = 2;
217+
renderWithIntl(
218+
<DisplayTheme
219+
allowedThemeKeys={['denim', 'sapphire']}
220+
{...displayThemeOtherProps}
221+
/>,
222+
);
223+
224+
const sapphireTile = screen.getByTestId('theme_display_settings.sapphire.option');
225+
226+
fireEvent.press(sapphireTile);
227+
228+
jest.advanceTimersByTime(750);
229+
jest.useRealTimers();
230+
231+
await waitFor(() => {
232+
expect(savePreference).toHaveBeenCalledWith(
233+
expect.any(String),
234+
expect.arrayContaining([
235+
expect.objectContaining({
236+
category: 'theme',
237+
value: expect.stringContaining('"type":"Sapphire"'),
238+
}),
239+
]),
240+
);
241+
expect(savePreference).toHaveBeenCalledTimes(1);
242+
});
243+
244+
jest.useFakeTimers();
245+
jest.advanceTimersByTime(750);
246+
247+
const denimTile = screen.getByTestId('theme_display_settings.denim.option');
248+
249+
fireEvent.press(denimTile);
250+
251+
// firing denimTile will not cause the savePreference to be called again since we have the prevent double tap
252+
expect(savePreference).toHaveBeenCalledTimes(numOfSavePreferenceCalls);
253+
254+
jest.useRealTimers();
255+
});
256+
257+
it('should not allow user to select a theme rapidly', async () => {
258+
const numOfSavePreferenceCalls = 1;
259+
renderWithIntl(
260+
<DisplayTheme
261+
allowedThemeKeys={['denim', 'sapphire']}
262+
{...displayThemeOtherProps}
263+
/>,
264+
);
265+
266+
const sapphireTile = screen.getByTestId('theme_display_settings.sapphire.option');
267+
268+
fireEvent.press(sapphireTile);
269+
270+
await waitFor(() => {
271+
expect(savePreference).toHaveBeenCalledWith(
272+
expect.any(String),
273+
expect.arrayContaining([
274+
expect.objectContaining({
275+
category: 'theme',
276+
value: expect.stringContaining('"type":"Sapphire"'),
277+
}),
278+
]),
279+
);
280+
expect(savePreference).toHaveBeenCalledTimes(numOfSavePreferenceCalls);
281+
});
282+
283+
const denimTile = screen.getByTestId('theme_display_settings.denim.option');
284+
285+
fireEvent.press(denimTile);
286+
287+
// firing denimTile will not cause the savePreference to be called again since we have the prevent double tap
288+
expect(savePreference).toHaveBeenCalledTimes(numOfSavePreferenceCalls);
289+
});
290+
});

‎app/screens/settings/display_theme/display_theme.tsx‎

Lines changed: 18 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved.
22
// See LICENSE.txt for license information.
33

4-
import React, {useCallback, useMemo, useState} from 'react';
4+
import React, {useCallback, useState} from 'react';
55

66
import {savePreference} from '@actions/remote/preference';
77
import SettingContainer from '@components/settings/container';
@@ -10,6 +10,7 @@ import {useServerUrl} from '@context/server';
1010
import {useTheme} from '@context/theme';
1111
import useAndroidHardwareBackHandler from '@hooks/android_back_handler';
1212
import useDidUpdate from '@hooks/did_update';
13+
import {usePreventDoubleTap} from '@hooks/utils';
1314
import {popTopScreen} from '@screens/navigation';
1415

1516
import CustomTheme from './custom_theme';
@@ -26,52 +27,44 @@ type DisplayThemeProps = {
2627
const DisplayTheme = ({allowedThemeKeys, componentId, currentTeamId, currentUserId}: DisplayThemeProps) => {
2728
const serverUrl = useServerUrl();
2829
const theme = useTheme();
29-
const initialTheme = useMemo(() => theme, [/* dependency array should remain empty */]);
30-
const [newTheme, setNewTheme] = useState<string | undefined>(undefined);
30+
const [customTheme, setCustomTheme] = useState(theme.type?.toLowerCase() === 'custom' ? theme : undefined);
3131

3232
const close = () => popTopScreen(componentId);
3333

34-
const setThemePreference = useCallback(() => {
35-
const allowedTheme = allowedThemeKeys.find((tk) => tk === newTheme);
36-
const themeJson = Preferences.THEMES[allowedTheme as ThemeKey] || initialTheme;
34+
const handleThemeChange = usePreventDoubleTap(useCallback(async (themeSelected: string) => {
35+
const allowedTheme = allowedThemeKeys.find((tk) => tk === themeSelected);
36+
const themeJson = Preferences.THEMES[allowedTheme as ThemeKey] || customTheme;
3737

3838
const pref: PreferenceType = {
3939
category: Preferences.CATEGORIES.THEME,
4040
name: currentTeamId,
4141
user_id: currentUserId,
4242
value: JSON.stringify(themeJson),
4343
};
44-
savePreference(serverUrl, [pref]);
45-
}, [allowedThemeKeys, initialTheme, currentTeamId, currentUserId, serverUrl, newTheme]);
44+
await savePreference(serverUrl, [pref]);
45+
}, [allowedThemeKeys, currentTeamId, currentUserId, customTheme, serverUrl]));
4646

4747
useDidUpdate(() => {
48-
const differentTheme = theme.type?.toLowerCase() !== newTheme?.toLowerCase();
49-
50-
if (!differentTheme) {
51-
close();
52-
return;
48+
// when the user selects any of the predefined theme when the current theme is custom, the custom theme will disappear.
49+
// by storing the current theme in the state, the custom theme will remain, and the user can switch back to it
50+
if (theme.type?.toLowerCase() === 'custom') {
51+
setCustomTheme(theme);
5352
}
54-
setThemePreference();
55-
}, [close, newTheme, setThemePreference, theme.type]);
56-
57-
const onAndroidBack = () => {
58-
setThemePreference();
59-
close();
60-
};
53+
}, [theme.type]);
6154

62-
useAndroidHardwareBackHandler(componentId, onAndroidBack);
55+
useAndroidHardwareBackHandler(componentId, close);
6356

6457
return (
6558
<SettingContainer testID='theme_display_settings'>
6659
<ThemeTiles
6760
allowedThemeKeys={allowedThemeKeys}
68-
onThemeChange={setNewTheme}
61+
onThemeChange={handleThemeChange}
6962
selectedTheme={theme.type}
7063
/>
71-
{initialTheme.type === 'custom' && (
64+
{customTheme && (
7265
<CustomTheme
73-
setTheme={setNewTheme}
74-
displayTheme={initialTheme.type}
66+
setTheme={handleThemeChange}
67+
displayTheme={'custom'}
7568
/>
7669
)}
7770
</SettingContainer>

0 commit comments

Comments
 (0)