diff --git a/packages/@react-aria/color/src/useColorField.ts b/packages/@react-aria/color/src/useColorField.ts index 2414b7b2ab7..70908094fea 100644 --- a/packages/@react-aria/color/src/useColorField.ts +++ b/packages/@react-aria/color/src/useColorField.ts @@ -85,7 +85,7 @@ export function useColorField( } else if (e.deltaY < 0) { decrement(); } - }, [isReadOnly, isDisabled, decrement, increment]); + }, [decrement, increment]); // If the input isn't supposed to receive input, disable scrolling. let scrollingDisabled = isDisabled || isReadOnly || !focusWithin; useScrollWheel({onScroll: onWheel, isDisabled: scrollingDisabled}, ref); diff --git a/packages/@react-aria/color/src/useColorWheel.ts b/packages/@react-aria/color/src/useColorWheel.ts index 9d171d98103..1c3ea44d93d 100644 --- a/packages/@react-aria/color/src/useColorWheel.ts +++ b/packages/@react-aria/color/src/useColorWheel.ts @@ -255,8 +255,8 @@ export function useColorWheel(props: ColorWheelAriaProps, state: ColorWheelState ...props, 'aria-label': ariaLabel }); - let step = stateRef.current.step; + let {minValue, maxValue, step} = state.value.getChannelRange('hue'); return { trackProps: { ...trackInteractions, @@ -300,8 +300,8 @@ export function useColorWheel(props: ColorWheelAriaProps, state: ColorWheelState inputLabellingProps, { type: 'range', - min: '0', - max: '360', + min: String(minValue), + max: String(maxValue), step: String(step), 'aria-valuetext': state.value.formatChannelValue('hue', locale), disabled: isDisabled, diff --git a/packages/@react-spectrum/color/stories/ColorArea.stories.tsx b/packages/@react-spectrum/color/stories/ColorArea.stories.tsx index 84095c4d688..fee910d1e92 100644 --- a/packages/@react-spectrum/color/stories/ColorArea.stories.tsx +++ b/packages/@react-spectrum/color/stories/ColorArea.stories.tsx @@ -84,7 +84,7 @@ XBlueYRed.storyName = 'RGB xChannel="blue", yChannel="red"'; XBlueYRed.args = {...XBlueYGreen.args, xChannel: 'blue', yChannel: 'red'}; export let XRedYBlue = Template.bind({}); -XRedYBlue.storyName = 'GB xChannel="red", yChannel="blue"'; +XRedYBlue.storyName = 'RGB xChannel="red", yChannel="blue"'; XRedYBlue.args = {...XBlueYGreen.args, xChannel: 'red', yChannel: 'blue'}; export let XRedYGreen = Template.bind({}); diff --git a/packages/@react-spectrum/color/stories/ColorField.stories.tsx b/packages/@react-spectrum/color/stories/ColorField.stories.tsx index ccd643eedb5..ee489d26f00 100644 --- a/packages/@react-spectrum/color/stories/ColorField.stories.tsx +++ b/packages/@react-spectrum/color/stories/ColorField.stories.tsx @@ -70,10 +70,6 @@ storiesOf('ColorField', module) 'with placeholder', () => render({placeholder: 'Enter a hex color'}) ) - .add( - 'step = 16', - () => render({step: 16}) - ) .add( 'controlled value', () => ( diff --git a/packages/@react-spectrum/color/stories/ColorSlider.stories.tsx b/packages/@react-spectrum/color/stories/ColorSlider.stories.tsx index 1954ff47504..5df71f60557 100644 --- a/packages/@react-spectrum/color/stories/ColorSlider.stories.tsx +++ b/packages/@react-spectrum/color/stories/ColorSlider.stories.tsx @@ -20,23 +20,19 @@ import {Text} from '@react-spectrum/text'; storiesOf('ColorSlider', module) .add( 'default', - () => + () => ) .add( 'no label, default aria-label', - () => + () => ) .add( 'no value label', - () => + () => ) .add( 'custom aria-label', - () => - ) - .add( - 'step', - () => + () => ) .add( 'disabled', @@ -52,11 +48,11 @@ storiesOf('ColorSlider', module) ) .add( 'custom width', - () => + () => ) .add( 'custom height', - () => + () => ) .add( 'rgba', diff --git a/packages/@react-spectrum/color/test/ColorField.test.js b/packages/@react-spectrum/color/test/ColorField.test.js index f2c8349f789..c29b37bc656 100644 --- a/packages/@react-spectrum/color/test/ColorField.test.js +++ b/packages/@react-spectrum/color/test/ColorField.test.js @@ -283,14 +283,13 @@ describe('ColorField', function () { it.each` Name | expected | key - ${'increment with arrow up key'} | ${parseColor('#AAAAAE')} | ${'ArrowUp'} - ${'decrement with arrow down key'} | ${parseColor('#AAAAA6')} | ${'ArrowDown'} + ${'increment with arrow up key'} | ${parseColor('#AAAAAB')} | ${'ArrowUp'} + ${'decrement with arrow down key'} | ${parseColor('#AAAAA9')} | ${'ArrowDown'} `('should handle $Name event', function ({expected, key}) { let onChangeSpy = jest.fn(); let {getByLabelText} = renderComponent({ defaultValue: '#aaa', - onChange: onChangeSpy, - step: 4 + onChange: onChangeSpy }); let colorField = getByLabelText('Primary Color'); expect(colorField.value).toBe('#AAAAAA'); @@ -303,14 +302,13 @@ describe('ColorField', function () { it.each` Name | expected | deltaY - ${'increment with mouse wheel'} | ${parseColor('#AAAAAE')} | ${10} - ${'decrement with mouse wheel'} | ${parseColor('#AAAAA6')} | ${-10} + ${'increment with mouse wheel'} | ${parseColor('#AAAAAB')} | ${10} + ${'decrement with mouse wheel'} | ${parseColor('#AAAAA9')} | ${-10} `('should handle $Name event', function ({expected, deltaY}) { let onChangeSpy = jest.fn(); let {getByLabelText} = renderComponent({ defaultValue: '#aaa', - onChange: onChangeSpy, - step: 4 + onChange: onChangeSpy }); let colorField = getByLabelText('Primary Color'); expect(colorField.value).toBe('#AAAAAA'); diff --git a/packages/@react-spectrum/color/test/ColorSlider.test.tsx b/packages/@react-spectrum/color/test/ColorSlider.test.tsx index 3504ec28333..067d42ca461 100644 --- a/packages/@react-spectrum/color/test/ColorSlider.test.tsx +++ b/packages/@react-spectrum/color/test/ColorSlider.test.tsx @@ -19,6 +19,7 @@ import userEvent from '@testing-library/user-event'; describe('ColorSlider', () => { let onChangeSpy = jest.fn(); + let onChangeEndSpy = jest.fn(); beforeAll(() => { jest.spyOn(window.HTMLElement.prototype, 'offsetWidth', 'get').mockImplementation(() => 100); @@ -31,6 +32,7 @@ describe('ColorSlider', () => { afterEach(() => { onChangeSpy.mockClear(); + onChangeEndSpy.mockClear(); // for restoreTextSelection act(() => {jest.runAllTimers();}); }); @@ -264,16 +266,65 @@ describe('ColorSlider', () => { describe('keyboard events', () => { it('works', () => { let defaultColor = parseColor('#000000'); - let {getByRole} = render(); + let {getByRole} = render(); let slider = getByRole('slider'); act(() => {slider.focus();}); fireEvent.keyDown(slider, {key: 'Right'}); + act(() => {jest.runAllTimers();}); expect(onChangeSpy).toHaveBeenCalledTimes(1); expect(onChangeSpy.mock.calls[0][0].toString('hexa')).toBe(defaultColor.withChannelValue('red', 1).toString('hexa')); + expect(onChangeEndSpy).toHaveBeenCalledTimes(1); + expect(onChangeEndSpy.mock.calls[0][0].toString('hexa')).toBe(defaultColor.withChannelValue('red', 1).toString('hexa')); + fireEvent.keyDown(slider, {key: 'Left'}); + act(() => {jest.runAllTimers();}); expect(onChangeSpy).toHaveBeenCalledTimes(2); expect(onChangeSpy.mock.calls[1][0].toString('hexa')).toBe(defaultColor.withChannelValue('red', 0).toString('hexa')); + expect(onChangeEndSpy).toHaveBeenCalledTimes(2); + expect(onChangeEndSpy.mock.calls[1][0].toString('hexa')).toBe(defaultColor.withChannelValue('red', 0).toString('hexa')); + + fireEvent.keyDown(slider, {key: 'PageUp'}); + act(() => {jest.runAllTimers();}); + expect(onChangeSpy).toHaveBeenCalledTimes(3); + expect(onChangeSpy.mock.calls[2][0].toString('hexa')).toBe(defaultColor.withChannelValue('red', 17).toString('hexa')); + expect(onChangeEndSpy).toHaveBeenCalledTimes(3); + expect(onChangeEndSpy.mock.calls[2][0].toString('hexa')).toBe(defaultColor.withChannelValue('red', 17).toString('hexa')); + + fireEvent.keyDown(slider, {key: 'Right'}); + act(() => {jest.runAllTimers();}); + expect(onChangeSpy).toHaveBeenCalledTimes(4); + expect(onChangeSpy.mock.calls[3][0].toString('hexa')).toBe(defaultColor.withChannelValue('red', 18).toString('hexa')); + expect(onChangeEndSpy).toHaveBeenCalledTimes(4); + expect(onChangeEndSpy.mock.calls[3][0].toString('hexa')).toBe(defaultColor.withChannelValue('red', 18).toString('hexa')); + + fireEvent.keyDown(slider, {key: 'PageDown'}); + act(() => {jest.runAllTimers();}); + expect(onChangeSpy).toHaveBeenCalledTimes(5); + expect(onChangeSpy.mock.calls[4][0].toString('hexa')).toBe(defaultColor.withChannelValue('red', 1).toString('hexa')); + expect(onChangeEndSpy).toHaveBeenCalledTimes(5); + expect(onChangeEndSpy.mock.calls[4][0].toString('hexa')).toBe(defaultColor.withChannelValue('red', 1).toString('hexa')); + + fireEvent.keyDown(slider, {key: 'End'}); + act(() => {jest.runAllTimers();}); + expect(onChangeSpy).toHaveBeenCalledTimes(6); + expect(onChangeSpy.mock.calls[5][0].toString('hexa')).toBe(defaultColor.withChannelValue('red', 255).toString('hexa')); + expect(onChangeEndSpy).toHaveBeenCalledTimes(6); + expect(onChangeEndSpy.mock.calls[5][0].toString('hexa')).toBe(defaultColor.withChannelValue('red', 255).toString('hexa')); + + fireEvent.keyDown(slider, {key: 'PageDown'}); + act(() => {jest.runAllTimers();}); + expect(onChangeSpy).toHaveBeenCalledTimes(7); + expect(onChangeSpy.mock.calls[6][0].toString('hexa')).toBe(defaultColor.withChannelValue('red', 238).toString('hexa')); + expect(onChangeEndSpy).toHaveBeenCalledTimes(7); + expect(onChangeEndSpy.mock.calls[6][0].toString('hexa')).toBe(defaultColor.withChannelValue('red', 238).toString('hexa')); + + fireEvent.keyDown(slider, {key: 'Home'}); + act(() => {jest.runAllTimers();}); + expect(onChangeSpy).toHaveBeenCalledTimes(8); + expect(onChangeSpy.mock.calls[7][0].toString('hexa')).toBe(defaultColor.withChannelValue('red', 0).toString('hexa')); + expect(onChangeEndSpy).toHaveBeenCalledTimes(8); + expect(onChangeEndSpy.mock.calls[7][0].toString('hexa')).toBe(defaultColor.withChannelValue('red', 0).toString('hexa')); }); it('doesn\'t work when disabled', () => { @@ -287,20 +338,6 @@ describe('ColorSlider', () => { fireEvent.keyDown(slider, {key: 'Left'}); expect(onChangeSpy).toHaveBeenCalledTimes(0); }); - - it('respects step', () => { - let defaultColor = parseColor('#000000'); - let {getByRole} = render(); - let slider = getByRole('slider'); - act(() => {slider.focus();}); - - fireEvent.keyDown(slider, {key: 'Right'}); - expect(onChangeSpy).toHaveBeenCalledTimes(1); - expect(onChangeSpy.mock.calls[0][0].toString('hexa')).toBe(defaultColor.withChannelValue('red', 10).toString('hexa')); - fireEvent.keyDown(slider, {key: 'Left'}); - expect(onChangeSpy).toHaveBeenCalledTimes(2); - expect(onChangeSpy.mock.calls[1][0].toString('hexa')).toBe(defaultColor.withChannelValue('red', 0).toString('hexa')); - }); }); describe.each` @@ -385,21 +422,6 @@ describe('ColorSlider', () => { expect(document.activeElement).not.toBe(slider); }); - it('dragging the thumb respects the step', () => { - let defaultColor = parseColor('hsl(0, 100%, 50%)'); - let {getByRole} = render(); - let slider = getByRole('slider'); - let thumb = slider.parentElement; - - start(thumb, {pageX: 0}); - expect(onChangeSpy).toHaveBeenCalledTimes(0); - move(thumb, {pageX: 20}); - expect(onChangeSpy).toHaveBeenCalledTimes(1); - expect(onChangeSpy.mock.calls[0][0].toString('hsla')).toBe(defaultColor.withChannelValue('hue', 120).toString('hsla')); - end(thumb, {pageX: 20}); - expect(onChangeSpy).toHaveBeenCalledTimes(1); - }); - it('clicking and dragging on the track works', () => { let defaultColor = parseColor('hsl(0, 100%, 50%)'); let {getByRole} = render(); @@ -465,22 +487,5 @@ describe('ColorSlider', () => { expect(onChangeSpy).toHaveBeenCalledTimes(0); expect(document.activeElement).not.toBe(slider); }); - - it('clicking and dragging on the track respects the step', () => { - let defaultColor = parseColor('hsl(0, 100%, 50%)'); - let {getByRole} = render(); - let slider = getByRole('slider'); - let thumb = slider.parentElement; - let container = getByRole('group'); - - start(container, {pageX: 30}); - expect(onChangeSpy).toHaveBeenCalledTimes(1); - expect(onChangeSpy.mock.calls[0][0].toString('hsla')).toBe(defaultColor.withChannelValue('saturation', 25).toString('hsla')); - move(thumb, {pageX: 50}); - expect(onChangeSpy).toHaveBeenCalledTimes(2); - expect(onChangeSpy.mock.calls[1][0].toString('hsla')).toBe(defaultColor.withChannelValue('saturation', 50).toString('hsla')); - end(thumb, {pageX: 50}); - expect(onChangeSpy).toHaveBeenCalledTimes(2); - }); }); }); diff --git a/packages/@react-stately/color/src/useColorFieldState.ts b/packages/@react-stately/color/src/useColorFieldState.ts index def0bafcb79..4f48117edd4 100644 --- a/packages/@react-stately/color/src/useColorFieldState.ts +++ b/packages/@react-stately/color/src/useColorFieldState.ts @@ -14,7 +14,7 @@ import {Color, ColorFieldProps} from '@react-types/color'; import {parseColor} from './Color'; import {useColor} from './useColor'; import {useControlledState} from '@react-stately/utils'; -import {useEffect, useMemo, useRef, useState} from 'react'; +import {useMemo, useRef, useState} from 'react'; export interface ColorFieldState { /** @@ -63,11 +63,11 @@ export function useColorFieldState( props: ColorFieldProps ): ColorFieldState { let { - step = 1, value, defaultValue, onChange } = props; + let {step} = MIN_COLOR.getChannelRange('red'); let initialValue = useColor(value); let initialDefaultValue = useColor(defaultValue); @@ -85,9 +85,12 @@ export function useColorFieldState( } }; - useEffect(() => { + let prevValue = useRef(colorValue); + if (prevValue.current !== colorValue) { setInputValue(colorValue ? colorValue.toString('hex') : ''); - }, [colorValue, setInputValue]); + prevValue.current = colorValue; + } + let parsedValue = useMemo(() => { let color; diff --git a/packages/@react-stately/color/src/useColorSliderState.ts b/packages/@react-stately/color/src/useColorSliderState.ts index bf72e3ce974..7f3146d76c9 100644 --- a/packages/@react-stately/color/src/useColorSliderState.ts +++ b/packages/@react-stately/color/src/useColorSliderState.ts @@ -51,7 +51,7 @@ export function useColorSliderState(props: ColorSliderStateOptions): ColorSlider setColor(color.withChannelValue(channel, v)); }, onChangeEnd([v]) { - // onChange will have already been called with the right value, this is just to trigger onChangEnd + // onChange will have already been called with the right value, this is just to trigger onChangeEnd if (props.onChangeEnd) { props.onChangeEnd(color.withChannelValue(channel, v)); } diff --git a/packages/@react-stately/color/src/useColorWheelState.ts b/packages/@react-stately/color/src/useColorWheelState.ts index 04c8bc99aa2..47ec32c97cb 100644 --- a/packages/@react-stately/color/src/useColorWheelState.ts +++ b/packages/@react-stately/color/src/useColorWheelState.ts @@ -101,6 +101,8 @@ export function useColorWheelState(props: ColorWheelProps): ColorWheelState { let valueRef = useRef(value); valueRef.current = value; + let channelRange = value.getChannelRange('hue'); + let {minValue: minValueX, maxValue: maxValueX, step: step, pageSize: pageStep} = channelRange; let [isDragging, setDragging] = useState(false); let isDraggingRef = useRef(false).current; @@ -118,9 +120,6 @@ export function useColorWheelState(props: ColorWheelProps): ColorWheelState { } } - let channelRange = value.getChannelRange('hue'); - let {minValue: minValueX, maxValue: maxValueX, step: step, pageSize: pageStep} = channelRange; - return { value, step, @@ -139,12 +138,13 @@ export function useColorWheelState(props: ColorWheelProps): ColorWheelState { return angleToCartesian(value.getChannelValue('hue'), radius); }, increment(stepSize = 1) { - let newValue = hue + Math.max(stepSize, step); - if (newValue > maxValueX) { + let s = Math.max(stepSize, step); + let newValue = hue + s; + if (newValue >= maxValueX) { // Make sure you can always get back to 0. newValue = minValueX; } - setHue(newValue); + setHue(roundToStep(mod(newValue, 360), s)); }, decrement(stepSize = 1) { let s = Math.max(stepSize, step); @@ -153,7 +153,7 @@ export function useColorWheelState(props: ColorWheelProps): ColorWheelState { // |(previous step) - 0| < step size setHue(roundDown(360 / s) * s); } else { - setHue(hue - s); + setHue(roundToStep(mod(hue - s, 360), s)); } }, setDragging(isDragging) { diff --git a/packages/@react-stately/color/test/useColorFieldState.test.js b/packages/@react-stately/color/test/useColorFieldState.test.js index 4c22f5b942e..71236fe7fdc 100644 --- a/packages/@react-stately/color/test/useColorFieldState.test.js +++ b/packages/@react-stately/color/test/useColorFieldState.test.js @@ -42,20 +42,6 @@ describe('useColorFieldState tests', function () { expect(result.current.inputValue).toBe('#AABBCC'); }); - it.each` - action | props - ${'increment'} | ${{defaultValue: '#000008', step: 4}} - ${'decrement'} | ${{defaultValue: '#000010', step: 4}} - `('should $action', function ({action, props}) { - let {result} = renderHook(() => useColorFieldState(props)); - act(() => result.current[action]()); - expect(result.current.colorValue.getChannelValue('red')).toBe(0); - expect(result.current.colorValue.getChannelValue('green')).toBe(0); - expect(result.current.colorValue.getChannelValue('blue')).toBe(12); - expect(result.current.colorValue.getChannelValue('alpha')).toBe(1); - expect(result.current.inputValue).toBe('#00000C'); - }); - it.each` name | action | props ${'not increment beyond max value'} | ${'increment'} | ${{defaultValue: '#ffffff'}} diff --git a/packages/@react-types/color/src/index.d.ts b/packages/@react-types/color/src/index.d.ts index 77ff433c084..647ca457150 100644 --- a/packages/@react-types/color/src/index.d.ts +++ b/packages/@react-types/color/src/index.d.ts @@ -80,12 +80,7 @@ export interface Color { export interface ColorFieldProps extends Omit, 'onChange'>, InputBase, Validation, FocusableProps, TextInputBase, LabelableProps { /** Handler that is called when the value changes. */ - onChange?: (color: Color | null) => void, - /** - * The step value to increment and decrement the color by when using the arrow keys. - * @default 1 - */ - step?: number + onChange?: (color: Color | null) => void } export interface AriaColorFieldProps extends ColorFieldProps, AriaLabelingProps, FocusableDOMProps, Omit, AriaValidationProps {} @@ -116,7 +111,7 @@ export interface SpectrumColorWheelProps extends AriaColorWheelProps, Omit