UI: remove material-ui-color dependency

[shyguy1412]

This PR will replace the current colorpicker from material-ui-color which is the last package thats holding us back from updating react

[shyguy1412]

TODO:

• Recreate colorpicker button
• Create colopicker modal
• add 'setColor' prop to colorpicker

[d0sboots]

Some high-level comments:

• This is very exciting! It looks like a lot of good work so far, hopefully a little more will get it across the finish line!
• It seems like you still need to hook it up in a few places?
• There's definitely some lint warnings to deal with. (npm run lint and npm run format will help you deal with that stuff mechanically)

[shyguy1412]

Yeah, I am currently struggling with time and mui.
Everything works fine mechanically but idk how to use mui to make the style apply.

If its ok I would just grab the theme and apply the style manually instead of using mui components x.x

The linting stuff ill take care of, I use a couple hooks in weird ways that the linter doesn't like. Mainly not always passing every dependency to optimize the amount of rerenders.

[d0sboots]

Do try to make the linter happy about hook dependencies, usually if it complains it means there's an edge case you haven't thought of. (Or you're really being too fancy, or haven't expressed other parts of the code properly.)

[shyguy1412]

This should be done now, some usability testing by other people would be helpful.

About the unused dependencies:

In HexInput, SVCanvas and RGBDigit the useMemo is acting as an onChange event.
These are some weird usages because of the need to sync state between sibling components.
the reason useMemo is used in the first place is because its run in sync, unlike useEffect which is run after the component has rendered. This is all to minimize the amount of rerenders the colorpicker needs to do to propagate state between sibling components. I'm open to better solution if you come up with one. For now this is both stable and efficient so I dont see an issue.

In index.ts (useDetachedState) useMemo is used as a "run once on mount"

[shyguy1412]

forgot about the ScriptEditor, should be good now

[Alpheus]

Found a console.log you forgot about.

[shyguy1412]

Ah, thanks! I missed that

[Alpheus]

What happens when this Error is thrown? Should we perhaps be more lenient and return a default color?

[shyguy1412]

This error shouldn't be reachable in production code. It only receives input from outside the component (current selected color) or already validated input from the HexInput. Both of those should have mechanisms already in place in case of invalid formatting so this is mainly to keep TS happy.
I'm against returning a default color because that makes it unclear when an error actually happens. If there is a case that throws this error that needs to be fixed, not hidden

[Alpheus]

If it shouldn't be reached then make sure it cannot be reached. Your last branch is checking length 7 and 9. What happens on the alpha check for hex[8] if the length is 7?

You can make the check exhaustive by padding the string with zeros then clamping it to length 9. That removes the need for the 2nd if (and error throw).

Suggested change

	function parseHEXtoRGB(hex: string): RGB {
	if (hex.length == 4) {
	return {
	r: Number.parseInt(`${hex[1]}${hex[1]}`, 16),
	g: Number.parseInt(`${hex[2]}${hex[2]}`, 16),
	b: Number.parseInt(`${hex[3]}${hex[3]}`, 16),
	};
	}
	if (hex.length == 7 || hex.length == 9) {
	return {
	r: Number.parseInt(`${hex[1]}${hex[2]}`, 16),
	g: Number.parseInt(`${hex[3]}${hex[4]}`, 16),
	b: Number.parseInt(`${hex[5]}${hex[6]}`, 16),
	a: Number.parseInt(`${hex[7]}${hex[8]}`, 16),
	};
	}
	throw new Error("Invalid hex string");
	function parseHEXtoRGB(hex: string): RGB {
	if (hex.length === 4) {
	return {
	r: Number.parseInt(`${hex[1]}${hex[1]}`, 16),
	g: Number.parseInt(`${hex[2]}${hex[2]}`, 16),
	b: Number.parseInt(`${hex[3]}${hex[3]}`, 16),
	};
	}
	hex = hex.padEnd(9, 'FF');
	return {
	r: Number.parseInt(`${hex[1]}${hex[2]}`, 16),
	g: Number.parseInt(`${hex[3]}${hex[4]}`, 16),
	b: Number.parseInt(`${hex[5]}${hex[6]}`, 16),
	a: Number.parseInt(`${hex[7]}${hex[8]}`, 16),
	};

[shyguy1412]

that could potentially lead to garbage if the hex string is actually wrongly formatted.
This function just parses a string to an RGB color, ensuring a correct input should be done wherever it is used.
This function must throw when the input is wrongly formatted and the consumer of this function must handle that case.
In our case this error is implicitly handled by the input validation for both the HexInput and onColorChange function

I agree on the first part tho, handling length 7 vs 9 should be done separately. That was an oversight on my part

[Alpheus]

Discussing offline in Discord. Expect some small nudges here.

[shyguy1412]

test probably failed randomly, needs a rerun

[Alpheus]

test probably failed randomly, needs a rerun

If it's flaky it's best to remove it.

[shyguy1412]

its a known issue unrelated to this PR so not much I can do

[d0sboots]

These are some weird usages because of the need to sync state between sibling components.
the reason useMemo is used in the first place is because its run in sync, unlike useEffect which is run after the component has rendered. This is all to minimize the amount of rerenders the colorpicker needs to do to propagate state between sibling components.

So, the canonical React answer here is: You don't propagate state between siblings. Instead, the state should be owned by the parent, which can then properly propagate it to its children.

[d0sboots]

The way this function is named (CapitalCamelCase), and the way it uses state, makes this a function component. That means it should be named accordingly: It is a Thing, not an Action. It should be called ColorPickerButton.

Edit: Perhaps you meant this to be a Thing, as in the Button that Opens the ColorPicker? But "Open" is not really needed here.

[d0sboots]

Already exists as clampNumber in utils/helpers.

[d0sboots]

I see what you are trying to do here with useDeferredState. But, it is a bad idea, and what you actually want to be doing is covered by other things that are builtin to React.

Apologies if I go over stuff you already know, but I want to be thorough here. Core to understanding what needs to happen here is understanding how React works, and then also understanding how React doesn't work, but can be made to handle.

Normally React operates by dealing with state. Every time state changes, React will rerender everything in that component, which recursively means all components below it. However, rerendering doesn't (necesarily) mean destroying and recreating DOM nodes, so typically this is pretty fast and doesn't cause any visible artifacts.

However, that approach doesn't work well for stuff like entering text in a textbox, or moving a slider. If we managed that with React state, then every single character would cause a rerender of the textbox, and every tiny nudge of the mouse would cause a rerender of the slider. The performance doesn't hold up.

For these scenarios, we fall back on native DOM functionality: we tell React to get the hell out of the way, we'll let the browser do its thing thank you. React calls this distinction "controlled vs uncontrolled" components: A controlled component is controlled with a state variable, and is always locked to the value of that state (via rerenders), while an uncontrolled component is not (the DOM will do its thing).

In your case, you are definitely trying to construct your overall component as an uncontrolled component, but you need to be using different approaches to do it. At the center of everything is the useRef method, which creates a mutable reference that lives with your component, and does not create rerenders. You can use the ref field in JSX to pass a ref object in, and it will get assigned the DOM object (as opposed to the React component), which lets you mutate the DOM in handlers. This is what you are actually trying to do in your mousedown on the SVCanvas, for instance.

I imagine when this is done you will mostly have various refs to specific DOM objects (the gradient, the textboxes) and very little actual state. setDetachedState and setSyncState will be gone.

See these pages for more details:
https://react.dev/learn/manipulating-the-dom-with-refs
https://legacy.reactjs.org/docs/uncontrolled-components.html
https://react.dev/reference/react-dom/components/input

[Alpheus]

Would you like help with the state management changes?