refactor: event editor color picker #1400
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Thank you very much for this. The code is clean and it follows project conventions. I also really appreciate the demo video. This is a great first contribution
I have left some small cleanup notes for you to consider.
I also noticed that the changes did not run through linter and you likely are missing a setup on your side
Asides from the cleanup suggestions, we need to revisit the debouncing logic to make sure we can provide good performance and UX to the feature
Feel free to reach out if I can help
apps/client/src/common/components/input/popover-picker/PopoverPicker.tsx
Outdated
Show resolved
Hide resolved
| <div className={style.inline}> | ||
| <PopoverPicker color={value} onChange={debouncedChange} icon={icon} hasInput={true} /> | ||
| <input type='hidden' name={name} value={value} /> | ||
| </div> | ||
| ); |
There was a problem hiding this comment.
question: I dont think we need the inline styles or the wrapper element at all since the input is not visible..
As far as I can test, a fragment here can suffice
<PopoverPicker color={value} onChange={debouncedChange} icon={icon} hasInput={true} />
apps/client/src/features/rundown/event-editor/composite/EventColorPicker.tsx
Outdated
Show resolved
Hide resolved
| const debouncedChange = debounceWithValue((value: string) => { | ||
| setColour(value); | ||
| }, 250); |
There was a problem hiding this comment.
we should keep in mind that changing the element colour invalidates the entire array list of elements
unfortunately this amount of debouncing is doesnt seem sufficient to
a) prevent issues with re-renders
b) provide a good user experience with the picker
I wonder if it would be possible for us to only submit on MouseUp event
There was a problem hiding this comment.
Maybe adding simple "OK/Cancel" buttons would be good?
There was a problem hiding this comment.
let me know which would be better. i've finished up on most of the tasks, was going to take this up next.
to use the onMouseUp event, will have to add the handler in the <PopoverPicker> component which is using onChange currently. If that's the behavior you're looking at other places, I'll add that
There was a problem hiding this comment.
I believe both options have its merits, MouseUp may be simpler while the buttons may be more explicit.
In my opinion, the UX of both options is equivalent and likely a matter of preference. For this reason I would favour the option that leads to the most elegant code
I will leave it up to you to make a proposal on this
There was a problem hiding this comment.
I believe directly using the color picker without adding buttons specifically for the <EventEditor> seems like a good user experience. As for debouncing, I figured out why the issue was happening.
I've changed the function signature to this and it works well with parameters now:
export function debounce(callback: (...args) => void, wait: number) {
let timeout: NodeJS.Timeout | null;
return (...args) => {
if (timeout) {
return;
}
timeout = setTimeout(() => {
timeout = null;
callback(...args);
}, wait);
};
}The issue that's occurring when we try to debounce is that the function is more of a throttle than a debounce. Happens to be a small but significant difference.
Upon changing the condition where if there's a timeout then return, to clearing the timeout, it's working well with the color picker.
export function debounce(callback: (...args) => void, wait: number) {
let timeout: NodeJS.Timeout | null;
return (...args) => {
if (timeout) {
clearTimeout(timeout);
}
timeout = setTimeout(() => {
timeout = null;
callback(...args);
}, wait);
};
}Would you like me to change the debounce function to this and allow it to apply everywhere it's currently being used or add a debounce and throttle function separately and make the respective changes where debounce was previously being used to throttle?
There was a problem hiding this comment.
Thank you for bringing this up.
I honestly always need to google throttle vs debounce as I tend to not be sure of the nuances
Throttle: Ensures that a function is called at most once in a specified time period.
Debounce: Ensures that a function is called only after a specified time period has passed since the last call.
In which case, you are correct that the existing function is a throttle with leading edge.
As for your question:
Would you like me to change the debounce function to this and allow it to apply everywhere it's currently being used or add a debounce and throttle function separately and make the respective changes where debounce was previously being used to throttle?
I believe the second option is the correct. We do not want to make changes to the existing features.
So we would prefer renaming the existing debounce to throttle and add a new debounce function as you suggest
There was a problem hiding this comment.
I honestly always need to google throttle vs debounce as I tend to not be sure of the nuances
Haha, same. Had to google it myself to be sure.
I'll get on this task. To give an update:
- The
<EventColorPicker>component is now<SwatchPicker>and I've put it under the respective directory too as specified. - I've added the
<SwatchPicker>component within the<SwatchSelect>component and it seems to have solved the wrapping issue too. - Added and removed styles accordingly as mentioned
- Fixed the linting issue as well, I believe. Had to run a
pnpm linton my end. Thanks for letting me know about that.
Once I'm done with this, should send in another push soon
apps/client/src/features/rundown/event-editor/composite/EventEditorTitles.tsx
Outdated
Show resolved
Hide resolved
apps/client/src/features/rundown/event-editor/composite/EventColorPicker.tsx
Outdated
Show resolved
Hide resolved
apps/client/src/common/components/input/popover-picker/PopoverPicker.module.scss
Outdated
Show resolved
Hide resolved
apps/client/src/common/components/input/popover-picker/PopoverPicker.module.scss
Outdated
Show resolved
Hide resolved
apps/client/src/features/rundown/event-editor/composite/EventEditorTitles.tsx
Outdated
Show resolved
Hide resolved
|
sure thing, i'll start off working on these changes and then ask for any opinion or guidance on the individual changes |
|
Hey, added the changed mentioned.
It was also mentioned that the icon class may not be required. I removed and tried to simply work with the swatch class itself but it wasn't giving the desired ui. The icon class mainly overrides some styles from the swatch class now. Let me know if any further changes are required |
|
Attaching a video for the recent changes: ontime-pr-2-2024-12-23_17.44.07.mp4 |
There was a problem hiding this comment.
Thank you for your work, really great.
I apologise for being slow in providing feedback at this time of year
I have left some small nitpicks for your consideration, only requested change is to fix the typing for the throttle and debounce functions.
It also seems that you have merge conflicts with master which we would need to resolve
apps/client/src/common/components/input/colour-input/SwatchPicker.tsx
Outdated
Show resolved
Hide resolved
apps/client/src/common/components/input/colour-input/SwatchPicker.tsx
Outdated
Show resolved
Hide resolved
apps/client/src/common/components/input/popover-picker/PopoverPicker.module.scss
Outdated
Show resolved
Hide resolved
Not at all. I'm extremely grateful for the review and suggestions you're giving. I've learnt quite a few things through these changes, super grateful :) I'll add in the recent changes suggested, fix the conflicts and send in another push |
|
Do let me know if there's any change to be done on the function signature. I believe with TypeScript that's the way to do it, in case there's a better way, would love to know about it |
apps/client/src/common/components/input/popover-picker/PopoverPicker.tsx
Outdated
Show resolved
Hide resolved
|
|
||
| const classes = cx([style.swatch, isSelected ? style.selected : null, style.selectable]); | ||
|
|
||
| const iconColor = Color(color).isLight() && isSelected ? '#000000' : '#FFFFFF'; |
There was a problem hiding this comment.
The tests are correctly catching an issue where the code will crash if the colour is not a valid colour string (ie: a empty string)
This process needs to be caught, similarly to what we do in thegetAccessibleColour I linked earlier
There was a problem hiding this comment.
ah, interesting. thanks, added this on the recent push
|
sure thing. let me know how you'd like to proceed with that |
Merged! I appreciate your contribution, thank you again. We want to implement your changes to the colour picker in the So your |
I loved working on this. Huge thanks to you for the reviews and suggestions, they really helped.
Yes, of course! I'll raise another PR for this. Anything else other than this? |
We have a large-ish backlog with lots of react task, but you would need to build some more familiarity with the code base to be able to be productive |
Sure thing. Regarding the change on If we're thinking to replace the
Let me know which approach you'd like to take. Apologies for any delay on my end |
The params editor is based on an HTML form element, on submit we convert the values into parameters and push it to the window location. I believe these design elements are good and worth maintaining. With this in mind, I believe that replacing the |
Super interesting. I'm learning a lot honestly. My bad in case I've missed out something and thank you for telling me when I am missing out anything. Really grateful.
Made this change and raised a PR under #1412 |
* refactor: event editor color picker * refactor: throttle and debounce function signature
closes #1397
Attaching a video for the PR
ontime-pr-2024-12-21_01.47.26.mp4
Changes
<PopoverPicker>to take iniconandhasInputas optional props + added<HexColorInput>from react-colorful if hasInput is true<EventColorPicker>as a component which uses<PopoverPicker>passing in icon and hasInput propsdebounceWithValuefunction underutils/debounce<HexColorInput>to look consistent with other input fields presentHope this is what you had in mind, do let me know if there's any changes required :))
P.S: Super fun code base, super clean. Loved working on this