From 3b86b9e14a2f6aef848c0df2bb388671dbf0b23d Mon Sep 17 00:00:00 2001 From: Vicktor <79470910+Victor-Nyagudi@users.noreply.github.com> Date: Wed, 6 May 2026 21:45:20 +0300 Subject: [PATCH] refactor(color_input): migrate ColorInput to a function component (#33363) * feat(color_input): migrate ColorInput to function component * test(color_input): migrate tests from enzyme to react testing library * test(color_input): remove obsolete snapshots * test(color_input): update snapshots * test(color_input): update test * refactor(color_input): remove unnecessary state update in function body * Revert "refactor(color_input): remove unnecessary state update in function body" This reverts commit 2c7647a3e49075535f3bcfd810b3d2de8173a7a0. * Fix ColorInput tests * Simplify click outside handler By changing the button to always show the picker instead of toggling it and making it so that the click outside handler checks for clicks outside of the whole ColorInput, we can get rid of the setTimeout and the very specific timing needed for the click outside handler. * Wrap handleColorChange in useCallback * Update snapshot --------- Co-authored-by: Mattermost Build Co-authored-by: Harrison Healey --- .../__snapshots__/color_input.test.tsx.snap | 479 ------------------ .../__snapshots__/color_setting.test.tsx.snap | 6 + .../src/components/color_input.test.tsx | 152 +++--- .../channels/src/components/color_input.tsx | 229 ++++----- .../__snapshots__/color_chooser.test.tsx.snap | 2 + .../custom_theme_chooser.test.tsx.snap | 46 ++ 6 files changed, 222 insertions(+), 692 deletions(-) delete mode 100644 webapp/channels/src/components/__snapshots__/color_input.test.tsx.snap diff --git a/webapp/channels/src/components/__snapshots__/color_input.test.tsx.snap b/webapp/channels/src/components/__snapshots__/color_input.test.tsx.snap deleted file mode 100644 index 6f106e97536..00000000000 --- a/webapp/channels/src/components/__snapshots__/color_input.test.tsx.snap +++ /dev/null @@ -1,479 +0,0 @@ -// Jest Snapshot v1, https://jestjs.io/docs/snapshot-testing - -exports[`components/ColorInput should match snapshot, click on picker 1`] = ` -
-
- - - - -
-
-
-
- -
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
- -
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
- - -
-
-
-
-
- - - -
-
-
-
-
-
-
-
-`; - -exports[`components/ColorInput should match snapshot, init 1`] = ` -
-
- - - - -
-
-`; - -exports[`components/ColorInput should match snapshot, opened 1`] = ` -
-
- - - - -
-
-
-
- -
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
- -
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
- - -
-
-
-
-
- - - -
-
-
-
-
-
-
-
-`; - -exports[`components/ColorInput should match snapshot, toggle picker 1`] = ` -
-
- - - - -
-
-`; diff --git a/webapp/channels/src/components/admin_console/__snapshots__/color_setting.test.tsx.snap b/webapp/channels/src/components/admin_console/__snapshots__/color_setting.test.tsx.snap index 835f626afa1..3e35be13cfb 100644 --- a/webapp/channels/src/components/admin_console/__snapshots__/color_setting.test.tsx.snap +++ b/webapp/channels/src/components/admin_console/__snapshots__/color_setting.test.tsx.snap @@ -28,10 +28,12 @@ exports[`components/ColorSetting should match snapshot, all 1`] = ` /> @@ -76,10 +78,12 @@ exports[`components/ColorSetting should match snapshot, clicked on color setting /> @@ -161,10 +165,12 @@ exports[`components/ColorSetting should match snapshot, no help text 1`] = ` /> diff --git a/webapp/channels/src/components/color_input.test.tsx b/webapp/channels/src/components/color_input.test.tsx index 85e16ca2dd7..223fd36ed4b 100644 --- a/webapp/channels/src/components/color_input.test.tsx +++ b/webapp/channels/src/components/color_input.test.tsx @@ -1,11 +1,28 @@ // Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. // See LICENSE.txt for license information. -import React from 'react'; +import React, {useCallback, useState} from 'react'; -import {render, screen, fireEvent, userEvent} from 'tests/react_testing_utils'; +import {renderWithContext, userEvent, screen, act} from 'tests/react_testing_utils'; -import ColorInput from './color_input'; +import ColorInput, {type ColorInputProps} from './color_input'; + +function ColorInputWrapper({onChange, value: initialValue, ...otherProps}: ColorInputProps) { + const [value, setValue] = useState(initialValue); + + const handleChange = useCallback((value: string) => { + setValue(value); + onChange(value); + }, [onChange]); + + return ( + + ); +} describe('components/ColorInput', () => { const baseProps = { @@ -14,105 +31,68 @@ describe('components/ColorInput', () => { value: '#ffffff', }; - test('should match snapshot, init', () => { - const {container} = render( - , - ); + test('should hide color picker when first rendered', () => { + const {getByTestId} = renderWithContext(); - expect(container).toMatchSnapshot(); + const inputElement = getByTestId('color-inputColorValue'); + + expect(inputElement).toBeInTheDocument(); + expect(inputElement).toBeVisible(); + expect(screen.queryByTestId('color-popover')).not.toBeInTheDocument(); }); - test('should match snapshot, opened', async () => { - const {container} = render( - , - ); + test('should show color picker when color picker button is clicked', async () => { + const {getByTestId} = renderWithContext(); - await userEvent.click(container.querySelector('.input-group-addon')!); + const colorPickerToggleButton = getByTestId('color-togglerButton'); - expect(container).toMatchSnapshot(); + await userEvent.click(colorPickerToggleButton); + + const colorPopover = getByTestId('color-popover'); + + expect(colorPopover).toBeInTheDocument(); + expect(colorPopover).toBeVisible(); + expect(document.activeElement).toBe(getByTestId('color-inputColorValue')); + + await userEvent.click(document.body); + + expect(screen.queryByTestId('color-popover')).not.toBeInTheDocument(); }); - test('should match snapshot, toggle picker', async () => { - const {container} = render( - , - ); - await userEvent.click(container.querySelector('.input-group-addon')!); - await userEvent.click(container.querySelector('.input-group-addon')!); + test('should change color when the color picker is clicked', async () => { + const {getByTestId} = renderWithContext(); - expect(container).toMatchSnapshot(); - }); + await userEvent.click(getByTestId('color-togglerButton')); - test('should match snapshot, click on picker', async () => { - const {container} = render( - , - ); + const colorPopover = getByTestId('color-popover'); - await userEvent.click(container.querySelector('.input-group-addon')!); - await userEvent.click(container.querySelector('.color-popover')!); + await userEvent.click(colorPopover); - expect(container).toMatchSnapshot(); - }); - - test('should have match state on togglePicker', async () => { - const {container} = render( - , - ); - - // Initially picker should be closed (no color-popover) - expect(container.querySelector('.color-popover')).not.toBeInTheDocument(); - - // Click to open - await userEvent.click(container.querySelector('.input-group-addon')!); - expect(container.querySelector('.color-popover')).toBeInTheDocument(); - - // Click to close - await userEvent.click(container.querySelector('.input-group-addon')!); - expect(container.querySelector('.color-popover')).not.toBeInTheDocument(); - - // Click to open again - await userEvent.click(container.querySelector('.input-group-addon')!); - expect(container.querySelector('.color-popover')).toBeInTheDocument(); + expect(colorPopover).toBeInTheDocument(); + expect(baseProps.onChange).toHaveBeenCalledTimes(1); }); test('should keep what the user types in the textbox until blur', async () => { - let currentValue = '#ffffff'; - const onChange = jest.fn((value: string) => { - currentValue = value; + const {getByTestId} = renderWithContext(); + + const inputElement = getByTestId('color-inputColorValue'); + + await userEvent.clear(inputElement); + await userEvent.type(inputElement, '#abc'); + + expect(inputElement).toHaveValue('#abc'); + + // The RGB here is the equivalent of '#abc'. + expect(getByTestId('color-icon').style.backgroundColor).toBe('rgb(170, 187, 204)'); + + await act(() => { + inputElement.blur(); }); - const {container, rerender} = render( - , - ); + expect(document.activeElement).not.toBe(inputElement); + expect(inputElement).toHaveValue('#aabbcc'); - const input = screen.getByRole('textbox'); - const colorIcon = container.querySelector('.color-icon') as HTMLElement; - - // Simulate focus on input - fireEvent used because userEvent doesn't have direct focus/blur methods - fireEvent.focus(input); - - await userEvent.clear(input); - await userEvent.type(input, '#abc'); - expect(onChange).toHaveBeenLastCalledWith('#aabbcc'); - expect(input).toHaveValue('#abc'); - expect(colorIcon.style.backgroundColor).toBe('rgb(170, 187, 204)'); - - // Rerender with updated value prop (simulating parent component update) - rerender( - , - ); - - fireEvent.blur(input); - - // After blur, the input should show the normalized value - expect(input).toHaveValue('#aabbcc'); - expect(colorIcon.style.backgroundColor).toBe('rgb(170, 187, 204)'); + // The RGB value passed in the assertion is the equivalent of '#aabbcc'. + expect(getByTestId('color-icon').style.backgroundColor).toBe('rgb(170, 187, 204)'); }); }); diff --git a/webapp/channels/src/components/color_input.tsx b/webapp/channels/src/components/color_input.tsx index f305b02e1a5..c8764b955d8 100644 --- a/webapp/channels/src/components/color_input.tsx +++ b/webapp/channels/src/components/color_input.tsx @@ -1,184 +1,159 @@ // Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. // See LICENSE.txt for license information. -import React from 'react'; +import React, {useCallback, useEffect, useRef, useState} from 'react'; import {ChromePicker} from 'react-color'; import type {ColorResult} from 'react-color'; import tinycolor from 'tinycolor2'; -type Props = { +export interface ColorInputProps { id: string; onChange: (color: string) => void; value: string; isDisabled?: boolean; } -type State = { - focused: boolean; - isOpened: boolean; - value: string; -} +const ColorInput = ({ + id, + onChange: onChangeFromProps, + value: valueFromProps, + isDisabled, +}: ColorInputProps) => { + const container = useRef(null); + const colorInput = useRef(null); -export default class ColorInput extends React.PureComponent { - private colorPicker: React.RefObject; - private colorInput: React.RefObject; + const [isFocused, setIsFocused] = useState(false); + const [isOpened, setIsOpened] = useState(false); + const [valueFromState, setValueFromState] = useState(valueFromProps); - public constructor(props: Props) { - super(props); - this.colorPicker = React.createRef(); - this.colorInput = React.createRef(); - - this.state = { - focused: false, - isOpened: false, - value: props.value, - }; + if (!isFocused && valueFromProps !== valueFromState) { + setValueFromState(valueFromProps); } - static getDerivedStateFromProps(props: Props, state: State) { - if (!state.focused && props.value !== state.value) { - return { - value: props.value, - }; + useEffect(() => { + if (!isOpened) { + return () => {}; } - return null; - } - - public componentDidUpdate(prevProps: Props, prevState: State) { - const {isOpened: prevIsOpened} = prevState; - const {isOpened} = this.state; - - if (isOpened !== prevIsOpened) { - if (isOpened) { - document.addEventListener('click', this.checkClick, {capture: true}); - } else { - document.removeEventListener('click', this.checkClick); + const checkClick = (e: MouseEvent): void => { + if (!container.current || !container.current.contains(e.target as Element)) { + setIsOpened(false); } - } - } + }; - private checkClick = (e: MouseEvent): void => { - if (!this.colorPicker.current || !this.colorPicker.current.contains(e.target as Element)) { - this.setState({isOpened: false}); - } + document.addEventListener('mousedown', checkClick); + + return () => { + document.removeEventListener('mousedown', checkClick); + }; + }, [isOpened]); + + const togglePicker = () => { + colorInput.current?.focus(); + setIsOpened(true); }; - private togglePicker = () => { - if (!this.state.isOpened && this.colorInput.current) { - this.colorInput.current.focus(); - } - this.setState({isOpened: !this.state.isOpened}); - }; + const handleColorChange = useCallback((newColorData: ColorResult) => { + setIsFocused(false); + onChangeFromProps(newColorData.hex); + }, [onChangeFromProps]); - public handleColorChange = (newColorData: ColorResult) => { - this.setState({focused: false}); - this.props.onChange(newColorData.hex); - }; - - private onChange = (event: React.ChangeEvent) => { + const onChange = (event: React.ChangeEvent) => { const value = event.target.value; const color = tinycolor(value); const normalizedColor = '#' + color.toHex(); if (color.isValid()) { - this.props.onChange(normalizedColor); + onChangeFromProps(normalizedColor); } - this.setState({value}); + setValueFromState(value); }; - private onFocus = (event: React.FocusEvent): void => { - this.setState({ - focused: true, - }); + const onFocus = (event: React.FocusEvent): void => { + setIsFocused(true); if (event.target) { event.target.setSelectionRange(1, event.target.value.length); } }; - private onBlur = () => { - const value = this.state.value; + const onBlur = () => { + const value = valueFromState; const color = tinycolor(value); const normalizedColor = '#' + color.toHex(); if (color.isValid()) { - this.props.onChange(normalizedColor); + onChangeFromProps(normalizedColor); - this.setState({ - value: normalizedColor, - }); + setValueFromState(normalizedColor); } else { - this.setState({ - value: this.props.value, - }); + setValueFromState(valueFromProps); } - this.setState({ - focused: false, - }); + setIsFocused(false); }; - private onKeyDown = (event: React.KeyboardEvent) => { + const onKeyDown = (event: React.KeyboardEvent) => { // open picker on enter or space if (event.key === 'Enter' || event.key === ' ') { - this.togglePicker(); + togglePicker(); } }; - public render() { - const {id} = this.props; - const {isOpened, value} = this.state; + return ( +
+ + {!isDisabled && + + + + } + {isOpened && ( +
+ +
+ )} +
+ ); +}; - return ( -
- - {!this.props.isDisabled && - - - - } - {isOpened && ( -
- -
- )} -
- ); - } -} +export default React.memo(ColorInput); diff --git a/webapp/channels/src/components/user_settings/display/user_settings_theme/color_chooser/__snapshots__/color_chooser.test.tsx.snap b/webapp/channels/src/components/user_settings/display/user_settings_theme/color_chooser/__snapshots__/color_chooser.test.tsx.snap index 6865c380420..1c887dd8b2e 100644 --- a/webapp/channels/src/components/user_settings/display/user_settings_theme/color_chooser/__snapshots__/color_chooser.test.tsx.snap +++ b/webapp/channels/src/components/user_settings/display/user_settings_theme/color_chooser/__snapshots__/color_chooser.test.tsx.snap @@ -21,10 +21,12 @@ exports[`components/user_settings/display/ColorChooser should match, init 1`] = /> diff --git a/webapp/channels/src/components/user_settings/display/user_settings_theme/custom_theme_chooser/__snapshots__/custom_theme_chooser.test.tsx.snap b/webapp/channels/src/components/user_settings/display/user_settings_theme/custom_theme_chooser/__snapshots__/custom_theme_chooser.test.tsx.snap index 563929073b6..256e8805fc9 100644 --- a/webapp/channels/src/components/user_settings/display/user_settings_theme/custom_theme_chooser/__snapshots__/custom_theme_chooser.test.tsx.snap +++ b/webapp/channels/src/components/user_settings/display/user_settings_theme/custom_theme_chooser/__snapshots__/custom_theme_chooser.test.tsx.snap @@ -63,10 +63,12 @@ exports[`components/user_settings/display/CustomThemeChooser should match, init /> @@ -95,10 +97,12 @@ exports[`components/user_settings/display/CustomThemeChooser should match, init /> @@ -127,10 +131,12 @@ exports[`components/user_settings/display/CustomThemeChooser should match, init /> @@ -159,10 +165,12 @@ exports[`components/user_settings/display/CustomThemeChooser should match, init /> @@ -191,10 +199,12 @@ exports[`components/user_settings/display/CustomThemeChooser should match, init /> @@ -223,10 +233,12 @@ exports[`components/user_settings/display/CustomThemeChooser should match, init /> @@ -255,10 +267,12 @@ exports[`components/user_settings/display/CustomThemeChooser should match, init /> @@ -287,10 +301,12 @@ exports[`components/user_settings/display/CustomThemeChooser should match, init /> @@ -319,10 +335,12 @@ exports[`components/user_settings/display/CustomThemeChooser should match, init /> @@ -351,10 +369,12 @@ exports[`components/user_settings/display/CustomThemeChooser should match, init /> @@ -383,10 +403,12 @@ exports[`components/user_settings/display/CustomThemeChooser should match, init /> @@ -415,10 +437,12 @@ exports[`components/user_settings/display/CustomThemeChooser should match, init /> @@ -447,10 +471,12 @@ exports[`components/user_settings/display/CustomThemeChooser should match, init /> @@ -479,10 +505,12 @@ exports[`components/user_settings/display/CustomThemeChooser should match, init /> @@ -547,10 +575,12 @@ exports[`components/user_settings/display/CustomThemeChooser should match, init /> @@ -579,10 +609,12 @@ exports[`components/user_settings/display/CustomThemeChooser should match, init /> @@ -611,10 +643,12 @@ exports[`components/user_settings/display/CustomThemeChooser should match, init /> @@ -643,10 +677,12 @@ exports[`components/user_settings/display/CustomThemeChooser should match, init /> @@ -675,10 +711,12 @@ exports[`components/user_settings/display/CustomThemeChooser should match, init /> @@ -707,10 +745,12 @@ exports[`components/user_settings/display/CustomThemeChooser should match, init /> @@ -824,10 +864,12 @@ exports[`components/user_settings/display/CustomThemeChooser should match, init /> @@ -856,10 +898,12 @@ exports[`components/user_settings/display/CustomThemeChooser should match, init /> @@ -888,10 +932,12 @@ exports[`components/user_settings/display/CustomThemeChooser should match, init />