fix(@clayui/color-picker): Update hue slider to use html range input #5156

pat270 · 2022-10-24T23:13:58Z

I finally got this working with the range input. I used the onKeyUp and onPointerUp events to update the Color Picker. The only problem with this is the map's hue doesn't update while dragging the thumb. Since I have something working, I can probably get it to work with onChange only, just need to throttle or debounce the updates so it doesn't lag so much.

In 87331f6#diff-47b43a95fa966e5189545f95207e5d85c86bb0a2ac02963ec706530728d79af5R39-R41, I had to blur then re-focus the input because FF has a range input issue when using onPointerUp. It fails to update the input value for some reason. This problem doesn't exist when using onMouseUp.

matuzalemsteles

Hey @pat270 there are some things we need to think about before going ahead with this and maybe improve some things, more in relation to the implementation of React.

It also seems that we have some problems with Slider as you said, I had made some changes to try to understand but it seems that React has some problems with input range, I will continue researching more about this but maybe we will have to not use the input range but we can still make it accessible.

I'll leave some comments here about the improvements that can be made.

matuzalemsteles · 2022-10-28T00:15:30Z

packages/clay-color-picker/src/Hue.tsx


-	const removeListeners = () => {
-		selectorActive.current = false;
+	const [internalValue, setInternalValue] = useState<number>(value);


We don't need another state here, because we also have a state in the parent component, in which case we have access to them which are value and onChange it is easier to maintain a source of truth than to have multiple states and keep synchronizing them all, this is also bad because of unnecessary rerenders.

matuzalemsteles · 2022-10-28T00:17:58Z

packages/clay-color-picker/src/Hue.tsx


-	const {onPointerMove, setXY, x, y} = usePointerPosition(containerRef);
+	const {setXY, y} = usePointerPosition(containerRef);


We don't need this hook anymore and we don't need to set setXY anymore because we are no longer using them here. It used to be just used to get x information to deal with the position of the pointer, as we don't have it anymore, we don't need it anymore.

matuzalemsteles · 2022-10-28T00:18:52Z

packages/clay-color-picker/src/Hue.tsx

+		event.target.blur();
+
+		event.target.focus();


I don't understand very well why we need this 😅, could you explain it better?

I think there is a bug in FF input range with onPointerUp. It keeps dragging the slider thumb even when the left mouse button is released. This didn't happen when I used onMouseUp. The only way I could get it to work properly was blur then focus.

Hmm this is weird, I'll do some testing.

matuzalemsteles · 2022-10-28T00:19:39Z

packages/clay-color-picker/src/Hue.tsx

 	React.useEffect(() => {
-		if (containerRef.current) {
-			setXY({x: hueToX(value, containerRef.current), y});
-		}
+		setInternalValue(value);


As we are going to use the parent state value and onChange we no longer need this to synchronize the values.

I couldn't do this one due to #5156 (comment).

matuzalemsteles · 2022-10-28T00:21:56Z

packages/clay-color-picker/src/Editor.tsx

+				<ClayInput.Group>
+					<ClayInput.GroupItem>
+						<ClayInput
+							data-testid="hInput"
+							insetBefore
+							max="360"
+							min="0"
+							onChange={(event) => {
+								const value = event.target.value;
+
+								let newVal = Number(value);
+
+								if (newVal < 0) {
+									newVal = 0;
+								} else if (newVal > 360) {
+									newVal = 360;
+								}
+
+								onHueChange(Math.round(newVal));
+
+								onColorChange(tinycolor({h: newVal, s, v}));
+							}}
+							type="number"
+							value={hue}
+						/>
+						<ClayInput.GroupInsetItem before tag="label">
+							H
+						</ClayInput.GroupInsetItem>
+					</ClayInput.GroupItem>
+				</ClayInput.Group>


As we are dealing with internal components we can move this composition to the Hue component, it is easier to keep the responsibility organized. As it is not an internal component, there is no problem rendering the bar along with the input.

matuzalemsteles · 2022-10-28T00:24:06Z

packages/clay-color-picker/src/Hue.tsx

-		window.removeEventListener('pointermove', onPointerMove);
-		window.removeEventListener('pointerup', removeListeners);
-	};
+	const handleOnChangeEnd = (event: any) => {


Since we won't have the internal state anymore, just add the onChange callback as a property of the ClaySlider, we won't need that anymore because we will be directly updating the parent state instead of the internal one.

pat270 · 2022-10-28T14:49:00Z

@matuzalemsteles Thanks for the input, I'll update this pr with your suggestions.

The problem is with the onChange, onHueChange, etc functions. React ends up re-rendering the slider when the value changes and the mouse loses focus. I was thinking if we could run onHueChange in all the other components, but not in slider. It would work.

matuzalemsteles · 2022-10-28T17:18:12Z

The problem is with the onChange, onHueChange, etc functions. React ends up re-rendering the slider when the value changes and the mouse loses focus. I was thinking if we could run onHueChange in all the other components, but not in slider. It would work.

I see, I'm not sure if that would be the problem because internally the ClaySlider has the useInternalState state, so if changing the slider value it causes a re-render, we wouldn't be doing anything different, other than controlling the state would stay in the ColorPicker instead of the slider when we are using the value and onChange properties.

I had made some changes to just use Hue's onChange, but the behavior is very strange that it loses the "focus" of the range when the mouse goes outside the bar, this is strange because the focus is still in the range if you monitor the document.activeElement, it doesn't make much sense.

Screen.Recording.2022-10-28.at.12.16.15.PM.mov

pat270 · 2022-10-28T18:13:45Z

The recording was exactly what was happening to me. useState seems to make this a non issue. It's the way the onChange functions work in the Editor imo. I'm trying useState with the alpha slider. I'll send a pr once I get swatches updating and we can compare.

matuzalemsteles · 2022-11-04T21:42:41Z

@pat270 I think the most ideal thing here to go with this would be to use onChange to update the Hue state so we can see the editor update as the slider changes its value. I'm trying to investigate why the mouse is getting lost when trying to move the slider.

- Styles the clay-range element and resets default values output by the clay-range-input-variant so base styles aren't duplicated by variants

…thumb for range with progress - This has better support for older browsers - Use the browser default thumb for clay-range-progress-none

- Adds new hue range input using native input range - Adds alpha input using native input range - Increase max height of Clay Color Dropdown Menu

…nent and SF

…h range input

matuzalemsteles · 2022-12-20T23:50:34Z

Hey @pat270 I think we're going to have to refactor how we implement the ClaySlider to behave differently and that the mouse moves work correctly when the onChange changes, maybe the input[type=range] maybe it has to be just the thumb and we will have to control movement via JS so it doesn't bug when state update happens.

pat270 force-pushed the clay-5137 branch 3 times, most recently from fef1014 to 5445aef Compare October 25, 2022 19:28

matuzalemsteles reviewed Oct 28, 2022

View reviewed changes

pat270 force-pushed the clay-5137 branch 2 times, most recently from 5207588 to 31f8440 Compare October 31, 2022 21:58

pat270 mentioned this pull request Nov 3, 2022

feat(@clayui/color-picker): Convert to use 8 digit hex colors and add alpha slider #5174

Closed

pat270 added 12 commits November 8, 2022 14:31

feat(@clayui/css): Mixins adds clay-range-variant

c60938f

- Styles the clay-range element and resets default values output by the clay-range-input-variant so base styles aren't duplicated by variants

fix(@clayui/css): Range show clay-range-thumb instead of input range …

94752e9

…thumb for range with progress - This has better support for older browsers - Use the browser default thumb for clay-range-progress-none

feat(@clayui/css): Clay Color adds hue and alpha range inputs

7fcfd63

- Adds new hue range input using native input range - Adds alpha input using native input range - Increase max height of Clay Color Dropdown Menu

feat(@clayui/css): Cadmin Clay Color adds hue and alpha range inputs

176b50b

fix(@clayui/css): Clay Color Firefox thumb size should be 14px x 14px

20bc06c

fix(@clayui/color-picker): Update hue slider to use html range input

7c8ee40

feat(@clayui/color-picker): Adds number input to show Hue value

28ee9ae

fix(@clayui/color-picker): Add dependency @clayui/slider

7a50743

fix(@clayui/color-picker): Move Hue input group to Hue internal compo…

8332b5f

…nent and SF

chore: Update yarn.lock

908ccf1

chore(@clayui/color-picker): Update snapshot

26b91e6

chore(@clayui/color-picker): Update hue color change test to work wit…

b3b1dbe

…h range input

pat270 force-pushed the clay-5137 branch from b5ee732 to b3b1dbe Compare November 8, 2022 23:20

pat270 closed this May 4, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(@clayui/color-picker): Update hue slider to use html range input #5156

fix(@clayui/color-picker): Update hue slider to use html range input #5156

pat270 commented Oct 24, 2022

matuzalemsteles left a comment

matuzalemsteles Oct 28, 2022

matuzalemsteles Oct 28, 2022

matuzalemsteles Oct 28, 2022

pat270 Oct 28, 2022

matuzalemsteles Nov 4, 2022

matuzalemsteles Oct 28, 2022

pat270 Oct 31, 2022

matuzalemsteles Oct 28, 2022

matuzalemsteles Oct 28, 2022

pat270 commented Oct 28, 2022

matuzalemsteles commented Oct 28, 2022

pat270 commented Oct 28, 2022

matuzalemsteles commented Nov 4, 2022

matuzalemsteles commented Dec 20, 2022


		const {onPointerMove, setXY, x, y} = usePointerPosition(containerRef);
		const {setXY, y} = usePointerPosition(containerRef);

fix(@clayui/color-picker): Update hue slider to use html range input #5156

fix(@clayui/color-picker): Update hue slider to use html range input #5156

Conversation

pat270 commented Oct 24, 2022

matuzalemsteles left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pat270 commented Oct 28, 2022

matuzalemsteles commented Oct 28, 2022

pat270 commented Oct 28, 2022

matuzalemsteles commented Nov 4, 2022

matuzalemsteles commented Dec 20, 2022