Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feat: Percentage field color customization #692

Merged
merged 30 commits into from
Jul 5, 2022
Merged
Show file tree
Hide file tree
Changes from 24 commits
Commits
Show all changes
30 commits
Select commit Hold shift + click to select a range
8644367
Merge pull request #685 from rowyio/develop
shamsmosowi Jun 16, 2022
40e259e
Merge branch 'develop' into rc
notsidney Jun 16, 2022
ad586e7
Merge branch 'develop' into rc
shamsmosowi Jun 17, 2022
57e758d
Merge pull request #686 from rowyio/rc
shamsmosowi Jun 17, 2022
f9e5be7
feat(percentage-c11n): convert to table cell
Jun 22, 2022
4882048
feat(percentage-c11n): add logic to default configs
Jun 22, 2022
f8c4269
feat(percentage-c11n): add color picker to settings
Jun 22, 2022
976dd7f
feat(percentage-c11n): change default colors
Jun 22, 2022
9bd1a4c
feat(percentage-c11n): fix button text color
Jun 22, 2022
52964a8
feat(percentage-c11n): add labels to settings
Jun 22, 2022
d74ac13
feat(percentage-c11n): add preview section
Jun 22, 2022
8854965
feat(percentage-c11n): fix cache issues with debouncing
Jun 23, 2022
13db993
feat(percentage-c11n): add width responsiveness to color picker
Jun 23, 2022
c25f420
feat(percentage-c11n): fix responsiveness issues
Jun 25, 2022
5afd5c7
feat(percentage-c11n): add checkbox, refactor a little
Jun 25, 2022
7ca47fa
feat(percentage-c11n): convert data type to array
Jun 28, 2022
591ccb2
feat(percentage-c11n): refactor config states
Jun 29, 2022
5069a75
feat(percentage-c11n): fix defaults
Jun 29, 2022
531b996
feat(percentage-c11n): add basic cell without bg
Jun 29, 2022
aa8d086
feat(percentage-c11n): remove collapse
Jun 29, 2022
5161c7c
feat(percentage-c11n): refactor checkStates
Jun 29, 2022
8fac4b2
feat(percentage-c11n): add grid layout
Jun 29, 2022
21443cd
feat(percentage-c11n): chore conventions
Jun 30, 2022
9fd88b3
feat(percentage-c11n): add default theme color to sidedrawer
Jun 30, 2022
2c1fbca
remove redundant fragment
htuerker Jun 30, 2022
bb73673
fix text color in preview
htuerker Jun 30, 2022
18aa406
fix: change state to derived state
htuerker Jul 2, 2022
e2bfef2
fix: review suggestions
Jul 2, 2022
0bd6f63
fix: remove redundant change call
htuerker Jul 4, 2022
d015144
fix(percentage-c11n): remove redundant dependencies
Jul 4, 2022
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
88 changes: 88 additions & 0 deletions src/components/ColorPickerInput.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,88 @@
import {
useState,
useEffect,
useRef,
MutableRefObject,
useLayoutEffect,
} from "react";
import { Box, useTheme } from "@mui/material";

import { Color, ColorPicker } from "react-color-palette";
import { useDebouncedCallback } from "use-debounce";

const useResponsiveWidth = (): [
width: number,
setRef: MutableRefObject<HTMLElement | null>
] => {
const ref = useRef(null);
const [width, setWidth] = useState(0);

useLayoutEffect(() => {
if (!ref || !ref.current) {
return;
}
const resizeObserver = new ResizeObserver((targets) => {
const { width: currentWidth } = targets[0].contentRect;
setWidth(currentWidth);
});

resizeObserver.observe(ref.current);

return () => {
resizeObserver.disconnect();
};
}, []);

return [width, ref];
};

export interface IColorPickerProps {
value: Color;
handleOnChangeComplete: (color: Color) => void;
disabled?: boolean;
}

export default function ColorPickerInput({
value,
handleOnChangeComplete,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

disabled = false,
}: IColorPickerProps) {
const [localValue, setLocalValue] = useState(value);
const [width, setRef] = useResponsiveWidth();
const theme = useTheme();
const debouncedOnChangeComplete = useDebouncedCallback((color) => {
handleOnChangeComplete(color);
}, 100);

useEffect(() => {
debouncedOnChangeComplete(localValue);
}, [debouncedOnChangeComplete, localValue]);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I鈥檓 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鈥檚 startTransition

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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鈥檛 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鈥檚 all local, so we don鈥檛 have to add a debounce.


Additionally, now that you鈥檝e accepted the suggestion to pass onChangeComplete directly, you鈥檝e 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鈥檝e added another change request and when you click accept on that in GitHub, I鈥檒l merge this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is obviously better!


return (
<Box
ref={setRef}
sx={[
{
padding: theme.spacing(1.5),
transitionDuration: 0,
"& .rcp": {
border: "none",
"& .rcp-saturation": {
borderRadius: theme.spacing(0.5),
},
"& .rcp-body": {
boxSizing: "unset",
htuerker marked this conversation as resolved.
Show resolved Hide resolved
},
},
},
]}
>
<ColorPicker
width={width}
height={150}
color={localValue}
onChange={(color) => setLocalValue(color)}
htuerker marked this conversation as resolved.
Show resolved Hide resolved
/>
</Box>
);
}
48 changes: 15 additions & 33 deletions src/components/fields/Percentage/BasicCell.tsx
Original file line number Diff line number Diff line change
@@ -1,41 +1,23 @@
import { IBasicCellProps } from "@src/components/fields/types";

import { useTheme } from "@mui/material";
import { resultColorsScale } from "@src/utils/color";

export default function Percentage({ value }: IBasicCellProps) {
const theme = useTheme();

if (typeof value === "number")
return (
<>
<div
style={{
backgroundColor: resultColorsScale(value).toHex(),

position: "absolute",
top: 0,
right: 0,
bottom: 0,
left: 0,
opacity: 0.5,

zIndex: 0,
}}
/>
<div
style={{
textAlign: "right",
color: theme.palette.text.primary,

position: "relative",
zIndex: 1,
}}
>
{Math.round(value * 100)}%
</div>
</>
);

return null;
const percentage = typeof value === "number" ? value : 0;
return (
<>
<div
style={{
textAlign: "right",
color: theme.palette.text.primary,
position: "relative",
zIndex: 1,
}}
>
{Math.round(percentage * 100)}%
</div>
</>
htuerker marked this conversation as resolved.
Show resolved Hide resolved
);
}
186 changes: 186 additions & 0 deletions src/components/fields/Percentage/Settings.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,186 @@
import { useEffect, useState } from "react";

import {
Box,
Checkbox,
Grid,
InputLabel,
MenuItem,
TextField,
Typography,
useTheme,
} from "@mui/material";
import ColorPickerInput from "@src/components/ColorPickerInput";
import { ISettingsProps } from "@src/components/fields/types";

import { Color, toColor } from "react-color-palette";
import { fieldSx } from "@src/components/SideDrawer/utils";
import { resultColorsScale, defaultColors } from "@src/utils/color";
import { GlobalStyles } from "tss-react";

const colorLabels: { [key: string]: string } = {
0: "Start",
1: "Middle",
2: "End",
};

export default function Settings({ onChange, config }: ISettingsProps) {
const [colorsDraft, setColorsDraft] = useState<any[]>(
config.colors ? config.colors : defaultColors
);

const [checkStates, setCheckStates] = useState<boolean[]>([
colorsDraft[0],
colorsDraft[1],
colorsDraft[2],
]);

useEffect(() => {
onChange("colors")(colorsDraft);
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [colorsDraft]);

const onCheckboxChange = (index: number, checked: boolean) => {
setColorsDraft(
colorsDraft.map((value: any, idx: number) =>
index === idx ? (checked ? value || defaultColors[idx] : null) : value
)
);
setCheckStates(
checkStates.map((value, idx) => (index === idx ? checked : value))
);
};

const handleColorChange = (index: number, color: Color): void => {
setColorsDraft(
colorsDraft.map((value, idx) => (index === idx ? color.hex : value))
);
};
htuerker marked this conversation as resolved.
Show resolved Hide resolved

return (
<>
<GlobalStyles
styles={{
"& .MuiList-root.MuiMenu-list": {
padding: 0,
},
}}
/>
htuerker marked this conversation as resolved.
Show resolved Hide resolved
<Grid container>
{checkStates.map((checked: boolean, index: number) => {
const colorHex = colorsDraft[index];
return (
<Grid
xs={12}
md={4}
item
sx={{
htuerker marked this conversation as resolved.
Show resolved Hide resolved
display: "flex",
alignItems: "end",
justifyContent: "center",
}}
>
<Checkbox
checked={checked}
sx={[
fieldSx,
{
width: "auto",
boxShadow: "none",
backgroundColor: "inherit",
"&:hover": {
backgroundColor: "inherit",
},
},
]}
onChange={() => onCheckboxChange(index, !checked)}
/>
<TextField
select
label={colorLabels[index]}
value={1}
fullWidth
disabled={!checkStates[index]}
htuerker marked this conversation as resolved.
Show resolved Hide resolved
>
<MenuItem value={1} sx={{ display: "none" }}>
{checked && (
<Box sx={{ display: "flex", alignItems: "center" }}>
<Box
sx={{
backgroundColor: colorHex,
width: 15,
height: 15,
mr: 1.5,
boxShadow: (theme) =>
`0 0 0 1px ${theme.palette.divider} inset`,
borderRadius: 0.5,
opacity: 0.5,
}}
/>
<Box>{colorHex}</Box>
</Box>
)}
</MenuItem>
{colorHex && (
<div>
<ColorPickerInput
value={toColor("hex", colorHex)}
handleOnChangeComplete={(color) =>
handleColorChange(index, color)
}
disabled={!checkStates[index]}
/>
</div>
)}
</TextField>
</Grid>
);
})}
</Grid>
<Preview colors={config.colors} />
</>
);
}

const Preview = ({ colors }: { colors: any }) => {
const theme = useTheme();
return (
<InputLabel>
Preview:
<Box
sx={{
display: "flex",
textAlign: "center",
}}
>
{[0, 0.125, 0.25, 0.375, 0.5, 0.625, 0.75, 0.875, 1].map((value) => {
return (
<Box
sx={{
position: "relative",
width: "100%",
padding: "0.5rem 0",
color: theme.palette.text.primary,
}}
>
<Box
key={value}
htuerker marked this conversation as resolved.
Show resolved Hide resolved
sx={{
position: "absolute",
inset: 0,
backgroundColor: resultColorsScale(
value,
colors,
theme.palette.background.paper
).toHex(),
opacity: 0.5,
}}
/>
<Typography>{Math.floor(value * 100)}%</Typography>
htuerker marked this conversation as resolved.
Show resolved Hide resolved
</Box>
);
})}
</Box>
</InputLabel>
);
};
14 changes: 9 additions & 5 deletions src/components/fields/Percentage/SideDrawerField.tsx
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
import { ISideDrawerFieldProps } from "@src/components/fields/types";

import { TextField, InputAdornment, Box } from "@mui/material";
import { emphasize } from "@mui/material/styles";
import { TextField, InputAdornment, Box, useTheme } from "@mui/material";
import { resultColorsScale } from "@src/utils/color";
import { getFieldId } from "@src/components/SideDrawer/utils";

Expand All @@ -12,6 +11,8 @@ export default function Percentage({
onSubmit,
disabled,
}: ISideDrawerFieldProps) {
const { colors } = (column as any).config;
const theme = useTheme();
return (
<TextField
variant="filled"
Expand All @@ -34,11 +35,14 @@ export default function Percentage({
width: 20,
height: 20,
borderRadius: 0.5,
boxShadow: (theme) =>
`0 0 0 1px ${theme.palette.divider} inest`,
boxShadow: `0 0 0 1px ${theme.palette.divider} inest`,
backgroundColor:
typeof value === "number"
? resultColorsScale(value).toHex() + "!important"
? resultColorsScale(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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鈥檛 need to change the boxShadow definition here or use useTheme

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

value,
colors,
theme.palette.background.paper
).toHex() + "!important"
: undefined,
}}
/>
Expand Down
Loading