Feat: Percentage field color customization

[htuerker]

Hello! 👋

This fixes #518.

Changes I made in order:

1. First, to meet the config requirement, I converted the Percentage field from BasicCell to TableCell.
2. I stored color configs like below:

config: {
   startColor: { hex, hsl, rgb } as Color;
   midColor: { hex, hsl, rgb } as Color;
   endColor: { hex, hsl, rgb } as Color;
}

3. I almost entirely copied the color picker logic from fields/Color/SideDrawerField
4. I created Settings and add it to the fields config as required.
5. I also extracted the ColorPickerInput for future usage i.e Toggle field - Add customization options #517 Slider Field - Add customization option #519. As I copied it from Color field, maybe we can try to use it there too.
6. There's no broken part against the new resultColorsScale utility logic as I handled this by giving a default colors parameter. Everything works fine.
7. I fixed emotion-cache problems by debouncing the dynamic style changes with 8854965
8. I added width responsiveness to ColorPicker container with fce32f4

I'm pretty much new to the codebase. I'd love to get a little more informative review. I'll be able to work on possible change requests.

Change previews:

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Ignored Deployment

Name	Status	Preview	Updated
rowy-typedoc	⬜️ Ignored (Inspect)		Jun 25, 2022 at 6:09AM (UTC)

[tinacai97]

Hi @htuerker! I'm Tina, the designer here at Rowy.
We're so glad to see you contributing to our project, thank you!

From a design standpoint, I have a couple of minor comments.

1. The color picker "appears" to be cut off. I don't think it actually is, but there seems to be some sort of illusion of cut-off because the bottom of the drop down has a rounded corner, while the top appears to stem straight from the drop down bar. I wonder if we could distance the color picker from the drop down and add some padding and a rounded corner... or whatever workaround you can think of!

2. I wonder if we can give users "optional" color sections. Perhaps I'd like two toned percentages, or maybe even one. It'd be cool to be able to have an easier option for that, rather than just setting the section I'd like "off" to fully transparent. Thought of checkboxes on the top of my head, but maybe you've got a better solution out there.

Thanks again for contributing to Rowy! We're an open-source community because of the awesome things people like you are able to add 😄

[tinacai97]

Hi @htuerker!

There's still an illusion on the color palette's both left and right borders. The only solution I can bring here is giving padding:

🥳 This is much clear-er! I definitely like the version with the padding.

My one tid-bit is if it would make more sense to keep the right angles at each corner of the actual color picker. I ask because aaaallll the way to the bottom-left would be black, #000, but would aaaallll the way to the top-left be white, #fff? I would imagine it might be slightly off white with the rounded corner there, though the hex code would tell me if that was the right assumption. But that also makes me wonder if there are some colors missing in those corners 🤔

Otherwise though, the color picker being encapsulated in the same rounded corner container is awesome!

I wonder if we can give users "optional" color sections. Perhaps I'd like two-toned percentages, or maybe even one.

I liked that idea. So, as default, we can simply set 3-colors(red-yellow-green). Then, leave it to the user to pick by checkboxes if wanted. Selecting from checkboxes would make it so easy to set single or double-toned variations.

💯 Perfect!

Thanks for your suggestions as well as a warm welcome! @tinacai97 ❤️

Of course, thank you for contributing! We hope you learned lots and got as much out of it as we did. I'll discuss pulling this with the team soon. Looking forward to any other contributions you make!

[shamsmosowi]

this is looking pretty cool! , instead of start/end colors, what about min/max colors?

[notsidney]

Hi @htuerker, this is a cracking PR! I work primarily on Rowy’s frontend and was the one who implemented the side drawer color picker you’re extending.

---

The only concern I have with this implementation is that you set the data structure for this config to be fixed to three items, start, mid, and end. I think it would be better to store it as an array of values instead, so we’re not constrained to only ever supporting 3 colors at most.

Then, the UI could be expanded in the future to support multiple color stops like a gradient editor in a design tool. Although the stops would be equidistant for simplicity.

Supporting more than 3 colors would require the resultColorsScale util to be rewritten to support an arbitrary number of colors, so we should hold off on allowing the user to set more than 3. We can update this config UI at any time, but if we stick with the current data structure, we’d need to “migrate” this config to the new one.

Additionally, if you store a CSS-compatible color in the config UI, like a hex or rgba (if supporting alpha) string, then you don’t need to import colord to convert it to a hex string in the table cell components.

---

For the config UI itself, I would suggest two changes:

1. Keep the text that displays the color’s hex code to be the standard text color (black in light mode, white in dark mode). I think you just need to unset the color CSS property. In the video you sent, when the color was set to yellow, the text would fail accessibility requirements for color contrast. The swatch on the left is enough to show the color selected.
2. If possible, it might be good to display the three color pickers side by side and make it responsive to save space. Those pickers feel a bit too wide. You can look at MUI’s Grid component for this. (Note: this is used to lay out using a 12-column grid, which predates CSS Grid. This component actually uses CSS Flex)

---

to meet the config requirement, I converted the Percentage field from BasicCell to TableCell

You can keep the BasicCell component, but remove the coloring for it, so that a value still appears on scroll, instead of displaying BasicCellNull.

---

And just a side note on the design for the color picker: it was meant to appear as one large rounded rectangle when opened, which is why the top corners of the react-color-palette UI are not rounded. This also removes the issue with parts of the color canvas being cut off by the rounded corners.

It appears that the styles are being overridden somewhere, or I could have copied the wrong styles, because the bottom corner radii don’t match the collapsed box (theme.shape.borderRadius * 1). Also, dark mode styles don’t seem to be working :/ This is what it’s meant to look like:

Ideally, it would closely match the design of other dropdown pickers like single and multi select, but those use a Popover. I think I found it easier to implement it as a collapsible element at the time.

That may also be a workaround to the issue with dynamic resizing—just set it to a fixed size all the time, like with the current date pickers (the MUI component isn’t easily resizable). The side drawer color picker is fixed at 440px, I believe.

---

I quite like the approach you took to bring out ColorPickerInput into its own component. That’s definitely the best way to do this, especially as you noted it will come in handy for future PRs to the other issues. I’m also quite impressed that you were able to understand our codebase, which at the moment isn’t very well documented!

[tinacai97]

Hi @htuerker,

Those changes look great! Except I might have not been clear with the color palette corners. I was trying to show in the image that we should keep them at right angles as to prevent any missing colors. See below:

Otherwise, it looks awesome to me! @notsidney can speak for the bugs and misbehaviors much better than I can.

[tinacai97]

And just a side note on the design for the color picker: it was meant to appear as one large rounded rectangle when opened, which is why the top corners of the react-color-palette UI are not rounded. This also removes the issue with parts of the color canvas being cut off by the rounded corners.

It appears that the styles are being overridden somewhere, or I could have copied the wrong styles, because the bottom corner radii don’t match the collapsed box (theme.shape.borderRadius * 1). Also, dark mode styles don’t seem to be working :/ This is what it’s meant to look like:

Hi @notsidney,

Just wanted to add here that the suggestion to separate the color picker from being directly adjacent to the drop down was by me 😅. The color picker felt a bit "cut off" when positioned there in my perspective. Additionally the actual circle indicator overlaps with some of the drop down when choosing a color at the top of the palette, as in your screenshot. So that is probably why some of the style changed and the color picker ended up getting some rounded corners.

Just wanted to add my two cents, I'll let you take it from here. I'm sure you know what is better design consistency wise!

[notsidney]

Hi @htuerker, to be clear, this is what we need to see to be able to merge this PR:

1. Change the data structure to be an array of CSS-compatible color values ['#fff', '#fa0', '#f00'] so we can expand on this in the future.
2. Reset the color of the hex code in the color picker to be the standard color, to keep it accessible.

The other comments I had I’ll leave to you to think about :)

[htuerker]

@notsidney I guess this is fine now! What a good first issue 😅

Last improvements:

1. Add BasicCell without background color on scroll instead BasicCellNull
2. Replace Collapse with TextField select
3. Add grid layout for picker checkboxes(md-4 sx-12)
4. Still favor a responsive color picker as a non-responsive one makes UI pretty ugly on the Color field's sidebar
5. For now, the overall state in Settings is hard-coded as we have the exact 3 inputs. But we can now easily expand it to support more colors.

[notsidney]

Just note that in the MUI sx prop, you can pass in a function with theme as the first argument to access the theme object for any CSS property, so you didn’t need to change the boxShadow definition here or use useTheme

[htuerker]

I did this change because I used the theme variable for the backgroundColor as well.

[notsidney]

Yes, I meant you could do the same thing for backgroundColor as well: backgroundColor: (theme) => ...

[notsidney]

I’m not sure why this is done this way. react-color-picker already has a onChangeComplete prop fired on mouseup. Then you can avoid debouncing and using an effect.

If you need to debounce (when writing to db), it should be handled in the parent component. If the reason for debounce is to improve performance, you can use React 18’s startTransition

Suggested change

	const debouncedOnChangeComplete = useDebouncedCallback((color) => {
	handleOnChangeComplete(color);
	}, 100);
	useEffect(() => {
	debouncedOnChangeComplete(localValue);
	}, [debouncedOnChangeComplete, localValue]);

[htuerker]

The problem here is every color changes frame(100 times+ just for a little drag) appends lots of styles into head as we're generating styles regarding. I'm aware of this problem for now. This also makes hard to reuse this component I already tried it for Color field.

I'm actually planning to work on another related issue after this. I've decided to think through this reusability problems during slider customization implementation. I'll research how we can use startTransition btw.

Also, a similar implementation used in Color field's PopoverCell.

[notsidney]

The problem here is every color changes frame(100 times+ just for a little drag)

It only re-renders this ColorPickerInput component, which is expected and necessary. This is why we use onChangeComplete to save the selected color only when the user has stopped interacting with the picker, and then update the other components like preview.

appends lots of styles into head as we're generating styles regarding.

This only happens when the user stops dragging (onChangeComplete), and MUI only appends styles to head in development mode. Notice that in dev mode, there are 1000+ nodes of <style data-emotion="mui" data-s> but these do not appear in production (like demo.rowy.io). So this isn’t a problem.

Also, a similar implementation used in Color field's PopoverCell.

This is because when onChangeComplete is called there, it immediately writes to the database, and we need to debounce writes for that since the user pays for writes. In this case, it’s all local, so we don’t have to add a debounce.

---

Additionally, now that you’ve accepted the suggestion to pass onChangeComplete directly, you’ve made this debounce + effect redundant. Adding logs, I saw that:

1. It fired when the dropdown is first opened.
2. It fired when the user stopped moving the mouse, but is still holding down the mouse button.
3. It fired when the user stopped holding the mouse button (this is the only time we want to fire), but since we already have an onChangeComplete prop, onChangeComplete is called twice at this point.

I’ve added another change request and when you click accept on that in GitHub, I’ll merge this PR.

[htuerker]

This is obviously better!

[notsidney]

Suggested change

	handleOnChangeComplete: (color: Color) => void;
	disabled?: boolean;
	}
	export default function ColorPickerInput({
	value,
	handleOnChangeComplete,
	onChangeComplete: ColorPickerProps['onChangeComplete'];
	disabled?: boolean;
	}
	export default function ColorPickerInput({
	value,
	onChangeComplete,

Pass the onChangeComplete prop to the <ColorPicker> component directly. You also need to import ColorPickerProps from react-color-palette.

[htuerker]

This is a nice trick I did not know it, thanks!

[htuerker]

I think it's better not to use this as onChangeComplete is optional in ColorPickerProps.

[notsidney]

I think it's better not to use this as onChangeComplete is optional in ColorPickerProps.

You can use NonNullable<ColorPickerProps['onChangeComplete']> (TypeScript docs)

[notsidney]

This is the last change required before I can merge this PR.