Skip to content

Commit 3c89231

Browse files
majornistamajornista
authored
fix(#2664): useColorSlider: refine PageUp/PageDown behaviors (#2666)
Get colorspace values from the actual colorspace instead of defaults Remove step from colorfield and colorslider Fix previous formatted value appearing quickly after blur
1 parent 8cef9a5 commit 3c89231

File tree

12 files changed

+86
-107
lines changed

12 files changed

+86
-107
lines changed

‎packages/@react-aria/color/src/useColorField.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -85,7 +85,7 @@ export function useColorField(
8585
} else if (e.deltaY < 0) {
8686
decrement();
8787
}
88-
}, [isReadOnly, isDisabled, decrement, increment]);
88+
}, [decrement, increment]);
8989
// If the input isn't supposed to receive input, disable scrolling.
9090
let scrollingDisabled = isDisabled || isReadOnly || !focusWithin;
9191
useScrollWheel({onScroll: onWheel, isDisabled: scrollingDisabled}, ref);

‎packages/@react-aria/color/src/useColorWheel.ts

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -255,8 +255,8 @@ export function useColorWheel(props: ColorWheelAriaProps, state: ColorWheelState
255255
...props,
256256
'aria-label': ariaLabel
257257
});
258-
let step = stateRef.current.step;
259258

259+
let {minValue, maxValue, step} = state.value.getChannelRange('hue');
260260
return {
261261
trackProps: {
262262
...trackInteractions,
@@ -300,8 +300,8 @@ export function useColorWheel(props: ColorWheelAriaProps, state: ColorWheelState
300300
inputLabellingProps,
301301
{
302302
type: 'range',
303-
min: '0',
304-
max: '360',
303+
min: String(minValue),
304+
max: String(maxValue),
305305
step: String(step),
306306
'aria-valuetext': state.value.formatChannelValue('hue', locale),
307307
disabled: isDisabled,

‎packages/@react-spectrum/color/stories/ColorArea.stories.tsx

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -84,7 +84,7 @@ XBlueYRed.storyName = 'RGB xChannel="blue", yChannel="red"';
8484
XBlueYRed.args = {...XBlueYGreen.args, xChannel: 'blue', yChannel: 'red'};
8585

8686
export let XRedYBlue = Template.bind({});
87-
XRedYBlue.storyName = 'GB xChannel="red", yChannel="blue"';
87+
XRedYBlue.storyName = 'RGB xChannel="red", yChannel="blue"';
8888
XRedYBlue.args = {...XBlueYGreen.args, xChannel: 'red', yChannel: 'blue'};
8989

9090
export let XRedYGreen = Template.bind({});

‎packages/@react-spectrum/color/stories/ColorField.stories.tsx

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -70,10 +70,6 @@ storiesOf('ColorField', module)
7070
'with placeholder',
7171
() => render({placeholder: 'Enter a hex color'})
7272
)
73-
.add(
74-
'step = 16',
75-
() => render({step: 16})
76-
)
7773
.add(
7874
'controlled value',
7975
() => (

‎packages/@react-spectrum/color/stories/ColorSlider.stories.tsx

Lines changed: 6 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -20,23 +20,19 @@ import {Text} from '@react-spectrum/text';
2020
storiesOf('ColorSlider', module)
2121
.add(
2222
'default',
23-
() => <ColorSlider defaultValue="#7f0000" channel={'red'} />
23+
() => <ColorSlider defaultValue="#800000" channel={'red'} />
2424
)
2525
.add(
2626
'no label, default aria-label',
27-
() => <ColorSlider defaultValue="#7f0000" channel={'red'} label={null} />
27+
() => <ColorSlider defaultValue="#800000" channel={'red'} label={null} />
2828
)
2929
.add(
3030
'no value label',
31-
() => <ColorSlider defaultValue="#7f0000" channel={'red'} showValueLabel={false} />
31+
() => <ColorSlider defaultValue="#800000" channel={'red'} showValueLabel={false} />
3232
)
3333
.add(
3434
'custom aria-label',
35-
() => <ColorSlider defaultValue="#7f0000" channel={'red'} aria-label="Color Picker Channel: Red" />
36-
)
37-
.add(
38-
'step',
39-
() => <ColorSlider defaultValue="hsl(0, 100%, 50%)" channel={'hue'} step={72} />
35+
() => <ColorSlider defaultValue="#800000" channel={'red'} aria-label="Color Picker Channel: Red" />
4036
)
4137
.add(
4238
'disabled',
@@ -52,11 +48,11 @@ storiesOf('ColorSlider', module)
5248
)
5349
.add(
5450
'custom width',
55-
() => <ColorSlider defaultValue="#7f0000" channel={'red'} width={300} />
51+
() => <ColorSlider defaultValue="#800000" channel={'red'} width={300} />
5652
)
5753
.add(
5854
'custom height',
59-
() => <ColorSlider defaultValue="#7f0000" channel={'red'} orientation="vertical" height={300} />
55+
() => <ColorSlider defaultValue="#800000" channel={'red'} orientation="vertical" height={300} />
6056
)
6157
.add(
6258
'rgba',

‎packages/@react-spectrum/color/test/ColorField.test.js

Lines changed: 6 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -283,14 +283,13 @@ describe('ColorField', function () {
283283

284284
it.each`
285285
Name | expected | key
286-
${'increment with arrow up key'} | ${parseColor('#AAAAAE')} | ${'ArrowUp'}
287-
${'decrement with arrow down key'} | ${parseColor('#AAAAA6')} | ${'ArrowDown'}
286+
${'increment with arrow up key'} | ${parseColor('#AAAAAB')} | ${'ArrowUp'}
287+
${'decrement with arrow down key'} | ${parseColor('#AAAAA9')} | ${'ArrowDown'}
288288
`('should handle $Name event', function ({expected, key}) {
289289
let onChangeSpy = jest.fn();
290290
let {getByLabelText} = renderComponent({
291291
defaultValue: '#aaa',
292-
onChange: onChangeSpy,
293-
step: 4
292+
onChange: onChangeSpy
294293
});
295294
let colorField = getByLabelText('Primary Color');
296295
expect(colorField.value).toBe('#AAAAAA');
@@ -303,14 +302,13 @@ describe('ColorField', function () {
303302

304303
it.each`
305304
Name | expected | deltaY
306-
${'increment with mouse wheel'} | ${parseColor('#AAAAAE')} | ${10}
307-
${'decrement with mouse wheel'} | ${parseColor('#AAAAA6')} | ${-10}
305+
${'increment with mouse wheel'} | ${parseColor('#AAAAAB')} | ${10}
306+
${'decrement with mouse wheel'} | ${parseColor('#AAAAA9')} | ${-10}
308307
`('should handle $Name event', function ({expected, deltaY}) {
309308
let onChangeSpy = jest.fn();
310309
let {getByLabelText} = renderComponent({
311310
defaultValue: '#aaa',
312-
onChange: onChangeSpy,
313-
step: 4
311+
onChange: onChangeSpy
314312
});
315313
let colorField = getByLabelText('Primary Color');
316314
expect(colorField.value).toBe('#AAAAAA');

‎packages/@react-spectrum/color/test/ColorSlider.test.tsx

Lines changed: 52 additions & 47 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@ import userEvent from '@testing-library/user-event';
1919

2020
describe('ColorSlider', () => {
2121
let onChangeSpy = jest.fn();
22+
let onChangeEndSpy = jest.fn();
2223

2324
beforeAll(() => {
2425
jest.spyOn(window.HTMLElement.prototype, 'offsetWidth', 'get').mockImplementation(() => 100);
@@ -31,6 +32,7 @@ describe('ColorSlider', () => {
3132

3233
afterEach(() => {
3334
onChangeSpy.mockClear();
35+
onChangeEndSpy.mockClear();
3436
// for restoreTextSelection
3537
act(() => {jest.runAllTimers();});
3638
});
@@ -264,16 +266,65 @@ describe('ColorSlider', () => {
264266
describe('keyboard events', () => {
265267
it('works', () => {
266268
let defaultColor = parseColor('#000000');
267-
let {getByRole} = render(<ColorSlider defaultValue={defaultColor} onChange={onChangeSpy} channel="red" />);
269+
let {getByRole} = render(<ColorSlider defaultValue={defaultColor} onChange={onChangeSpy} onChangeEnd={onChangeEndSpy} channel="red" />);
268270
let slider = getByRole('slider');
269271
act(() => {slider.focus();});
270272

271273
fireEvent.keyDown(slider, {key: 'Right'});
274+
act(() => {jest.runAllTimers();});
272275
expect(onChangeSpy).toHaveBeenCalledTimes(1);
273276
expect(onChangeSpy.mock.calls[0][0].toString('hexa')).toBe(defaultColor.withChannelValue('red', 1).toString('hexa'));
277+
expect(onChangeEndSpy).toHaveBeenCalledTimes(1);
278+
expect(onChangeEndSpy.mock.calls[0][0].toString('hexa')).toBe(defaultColor.withChannelValue('red', 1).toString('hexa'));
279+
274280
fireEvent.keyDown(slider, {key: 'Left'});
281+
act(() => {jest.runAllTimers();});
275282
expect(onChangeSpy).toHaveBeenCalledTimes(2);
276283
expect(onChangeSpy.mock.calls[1][0].toString('hexa')).toBe(defaultColor.withChannelValue('red', 0).toString('hexa'));
284+
expect(onChangeEndSpy).toHaveBeenCalledTimes(2);
285+
expect(onChangeEndSpy.mock.calls[1][0].toString('hexa')).toBe(defaultColor.withChannelValue('red', 0).toString('hexa'));
286+
287+
fireEvent.keyDown(slider, {key: 'PageUp'});
288+
act(() => {jest.runAllTimers();});
289+
expect(onChangeSpy).toHaveBeenCalledTimes(3);
290+
expect(onChangeSpy.mock.calls[2][0].toString('hexa')).toBe(defaultColor.withChannelValue('red', 17).toString('hexa'));
291+
expect(onChangeEndSpy).toHaveBeenCalledTimes(3);
292+
expect(onChangeEndSpy.mock.calls[2][0].toString('hexa')).toBe(defaultColor.withChannelValue('red', 17).toString('hexa'));
293+
294+
fireEvent.keyDown(slider, {key: 'Right'});
295+
act(() => {jest.runAllTimers();});
296+
expect(onChangeSpy).toHaveBeenCalledTimes(4);
297+
expect(onChangeSpy.mock.calls[3][0].toString('hexa')).toBe(defaultColor.withChannelValue('red', 18).toString('hexa'));
298+
expect(onChangeEndSpy).toHaveBeenCalledTimes(4);
299+
expect(onChangeEndSpy.mock.calls[3][0].toString('hexa')).toBe(defaultColor.withChannelValue('red', 18).toString('hexa'));
300+
301+
fireEvent.keyDown(slider, {key: 'PageDown'});
302+
act(() => {jest.runAllTimers();});
303+
expect(onChangeSpy).toHaveBeenCalledTimes(5);
304+
expect(onChangeSpy.mock.calls[4][0].toString('hexa')).toBe(defaultColor.withChannelValue('red', 1).toString('hexa'));
305+
expect(onChangeEndSpy).toHaveBeenCalledTimes(5);
306+
expect(onChangeEndSpy.mock.calls[4][0].toString('hexa')).toBe(defaultColor.withChannelValue('red', 1).toString('hexa'));
307+
308+
fireEvent.keyDown(slider, {key: 'End'});
309+
act(() => {jest.runAllTimers();});
310+
expect(onChangeSpy).toHaveBeenCalledTimes(6);
311+
expect(onChangeSpy.mock.calls[5][0].toString('hexa')).toBe(defaultColor.withChannelValue('red', 255).toString('hexa'));
312+
expect(onChangeEndSpy).toHaveBeenCalledTimes(6);
313+
expect(onChangeEndSpy.mock.calls[5][0].toString('hexa')).toBe(defaultColor.withChannelValue('red', 255).toString('hexa'));
314+
315+
fireEvent.keyDown(slider, {key: 'PageDown'});
316+
act(() => {jest.runAllTimers();});
317+
expect(onChangeSpy).toHaveBeenCalledTimes(7);
318+
expect(onChangeSpy.mock.calls[6][0].toString('hexa')).toBe(defaultColor.withChannelValue('red', 238).toString('hexa'));
319+
expect(onChangeEndSpy).toHaveBeenCalledTimes(7);
320+
expect(onChangeEndSpy.mock.calls[6][0].toString('hexa')).toBe(defaultColor.withChannelValue('red', 238).toString('hexa'));
321+
322+
fireEvent.keyDown(slider, {key: 'Home'});
323+
act(() => {jest.runAllTimers();});
324+
expect(onChangeSpy).toHaveBeenCalledTimes(8);
325+
expect(onChangeSpy.mock.calls[7][0].toString('hexa')).toBe(defaultColor.withChannelValue('red', 0).toString('hexa'));
326+
expect(onChangeEndSpy).toHaveBeenCalledTimes(8);
327+
expect(onChangeEndSpy.mock.calls[7][0].toString('hexa')).toBe(defaultColor.withChannelValue('red', 0).toString('hexa'));
277328
});
278329

279330
it('doesn\'t work when disabled', () => {
@@ -287,20 +338,6 @@ describe('ColorSlider', () => {
287338
fireEvent.keyDown(slider, {key: 'Left'});
288339
expect(onChangeSpy).toHaveBeenCalledTimes(0);
289340
});
290-
291-
it('respects step', () => {
292-
let defaultColor = parseColor('#000000');
293-
let {getByRole} = render(<ColorSlider defaultValue={defaultColor} onChange={onChangeSpy} channel="red" step={10} />);
294-
let slider = getByRole('slider');
295-
act(() => {slider.focus();});
296-
297-
fireEvent.keyDown(slider, {key: 'Right'});
298-
expect(onChangeSpy).toHaveBeenCalledTimes(1);
299-
expect(onChangeSpy.mock.calls[0][0].toString('hexa')).toBe(defaultColor.withChannelValue('red', 10).toString('hexa'));
300-
fireEvent.keyDown(slider, {key: 'Left'});
301-
expect(onChangeSpy).toHaveBeenCalledTimes(2);
302-
expect(onChangeSpy.mock.calls[1][0].toString('hexa')).toBe(defaultColor.withChannelValue('red', 0).toString('hexa'));
303-
});
304341
});
305342

306343
describe.each`
@@ -385,21 +422,6 @@ describe('ColorSlider', () => {
385422
expect(document.activeElement).not.toBe(slider);
386423
});
387424

388-
it('dragging the thumb respects the step', () => {
389-
let defaultColor = parseColor('hsl(0, 100%, 50%)');
390-
let {getByRole} = render(<ColorSlider channel="hue" defaultValue={defaultColor} onChange={onChangeSpy} step={120} />);
391-
let slider = getByRole('slider');
392-
let thumb = slider.parentElement;
393-
394-
start(thumb, {pageX: 0});
395-
expect(onChangeSpy).toHaveBeenCalledTimes(0);
396-
move(thumb, {pageX: 20});
397-
expect(onChangeSpy).toHaveBeenCalledTimes(1);
398-
expect(onChangeSpy.mock.calls[0][0].toString('hsla')).toBe(defaultColor.withChannelValue('hue', 120).toString('hsla'));
399-
end(thumb, {pageX: 20});
400-
expect(onChangeSpy).toHaveBeenCalledTimes(1);
401-
});
402-
403425
it('clicking and dragging on the track works', () => {
404426
let defaultColor = parseColor('hsl(0, 100%, 50%)');
405427
let {getByRole} = render(<ColorSlider channel="hue" defaultValue={defaultColor} onChange={onChangeSpy} />);
@@ -465,22 +487,5 @@ describe('ColorSlider', () => {
465487
expect(onChangeSpy).toHaveBeenCalledTimes(0);
466488
expect(document.activeElement).not.toBe(slider);
467489
});
468-
469-
it('clicking and dragging on the track respects the step', () => {
470-
let defaultColor = parseColor('hsl(0, 100%, 50%)');
471-
let {getByRole} = render(<ColorSlider channel="saturation" defaultValue={defaultColor} onChange={onChangeSpy} step={25} />);
472-
let slider = getByRole('slider');
473-
let thumb = slider.parentElement;
474-
let container = getByRole('group');
475-
476-
start(container, {pageX: 30});
477-
expect(onChangeSpy).toHaveBeenCalledTimes(1);
478-
expect(onChangeSpy.mock.calls[0][0].toString('hsla')).toBe(defaultColor.withChannelValue('saturation', 25).toString('hsla'));
479-
move(thumb, {pageX: 50});
480-
expect(onChangeSpy).toHaveBeenCalledTimes(2);
481-
expect(onChangeSpy.mock.calls[1][0].toString('hsla')).toBe(defaultColor.withChannelValue('saturation', 50).toString('hsla'));
482-
end(thumb, {pageX: 50});
483-
expect(onChangeSpy).toHaveBeenCalledTimes(2);
484-
});
485490
});
486491
});

‎packages/@react-stately/color/src/useColorFieldState.ts

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,7 @@ import {Color, ColorFieldProps} from '@react-types/color';
1414
import {parseColor} from './Color';
1515
import {useColor} from './useColor';
1616
import {useControlledState} from '@react-stately/utils';
17-
import {useEffect, useMemo, useRef, useState} from 'react';
17+
import {useMemo, useRef, useState} from 'react';
1818

1919
export interface ColorFieldState {
2020
/**
@@ -63,11 +63,11 @@ export function useColorFieldState(
6363
props: ColorFieldProps
6464
): ColorFieldState {
6565
let {
66-
step = 1,
6766
value,
6867
defaultValue,
6968
onChange
7069
} = props;
70+
let {step} = MIN_COLOR.getChannelRange('red');
7171

7272
let initialValue = useColor(value);
7373
let initialDefaultValue = useColor(defaultValue);
@@ -85,9 +85,12 @@ export function useColorFieldState(
8585
}
8686
};
8787

88-
useEffect(() => {
88+
let prevValue = useRef(colorValue);
89+
if (prevValue.current !== colorValue) {
8990
setInputValue(colorValue ? colorValue.toString('hex') : '');
90-
}, [colorValue, setInputValue]);
91+
prevValue.current = colorValue;
92+
}
93+
9194

9295
let parsedValue = useMemo(() => {
9396
let color;

‎packages/@react-stately/color/src/useColorSliderState.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -51,7 +51,7 @@ export function useColorSliderState(props: ColorSliderStateOptions): ColorSlider
5151
setColor(color.withChannelValue(channel, v));
5252
},
5353
onChangeEnd([v]) {
54-
// onChange will have already been called with the right value, this is just to trigger onChangEnd
54+
// onChange will have already been called with the right value, this is just to trigger onChangeEnd
5555
if (props.onChangeEnd) {
5656
props.onChangeEnd(color.withChannelValue(channel, v));
5757
}

‎packages/@react-stately/color/src/useColorWheelState.ts

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -101,6 +101,8 @@ export function useColorWheelState(props: ColorWheelProps): ColorWheelState {
101101
let valueRef = useRef(value);
102102
valueRef.current = value;
103103

104+
let channelRange = value.getChannelRange('hue');
105+
let {minValue: minValueX, maxValue: maxValueX, step: step, pageSize: pageStep} = channelRange;
104106
let [isDragging, setDragging] = useState(false);
105107
let isDraggingRef = useRef(false).current;
106108

@@ -118,9 +120,6 @@ export function useColorWheelState(props: ColorWheelProps): ColorWheelState {
118120
}
119121
}
120122

121-
let channelRange = value.getChannelRange('hue');
122-
let {minValue: minValueX, maxValue: maxValueX, step: step, pageSize: pageStep} = channelRange;
123-
124123
return {
125124
value,
126125
step,
@@ -139,12 +138,13 @@ export function useColorWheelState(props: ColorWheelProps): ColorWheelState {
139138
return angleToCartesian(value.getChannelValue('hue'), radius);
140139
},
141140
increment(stepSize = 1) {
142-
let newValue = hue + Math.max(stepSize, step);
143-
if (newValue > maxValueX) {
141+
let s = Math.max(stepSize, step);
142+
let newValue = hue + s;
143+
if (newValue >= maxValueX) {
144144
// Make sure you can always get back to 0.
145145
newValue = minValueX;
146146
}
147-
setHue(newValue);
147+
setHue(roundToStep(mod(newValue, 360), s));
148148
},
149149
decrement(stepSize = 1) {
150150
let s = Math.max(stepSize, step);
@@ -153,7 +153,7 @@ export function useColorWheelState(props: ColorWheelProps): ColorWheelState {
153153
// |(previous step) - 0| < step size
154154
setHue(roundDown(360 / s) * s);
155155
} else {
156-
setHue(hue - s);
156+
setHue(roundToStep(mod(hue - s, 360), s));
157157
}
158158
},
159159
setDragging(isDragging) {

‎packages/@react-stately/color/test/useColorFieldState.test.js

Lines changed: 0 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -42,20 +42,6 @@ describe('useColorFieldState tests', function () {
4242
expect(result.current.inputValue).toBe('#AABBCC');
4343
});
4444

45-
it.each`
46-
action | props
47-
${'increment'} | ${{defaultValue: '#000008', step: 4}}
48-
${'decrement'} | ${{defaultValue: '#000010', step: 4}}
49-
`('should $action', function ({action, props}) {
50-
let {result} = renderHook(() => useColorFieldState(props));
51-
act(() => result.current[action]());
52-
expect(result.current.colorValue.getChannelValue('red')).toBe(0);
53-
expect(result.current.colorValue.getChannelValue('green')).toBe(0);
54-
expect(result.current.colorValue.getChannelValue('blue')).toBe(12);
55-
expect(result.current.colorValue.getChannelValue('alpha')).toBe(1);
56-
expect(result.current.inputValue).toBe('#00000C');
57-
});
58-
5945
it.each`
6046
name | action | props
6147
${'not increment beyond max value'} | ${'increment'} | ${{defaultValue: '#ffffff'}}

0 commit comments

Comments
 (0)