Fix unexpected movements, and closing on first click in the color picker #35670
+12
−36
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jorgefilipecosta
requested review from
ajitbohra and
chrisvanpatten
as code owners
|
Size Change: +202 B (0%) Total Size: 1.07 MB
ℹ️ View Unchanged
|
jorgefilipecosta
force-pushed
the
update/fix-issue-on-color-picker-component-remove-tiny-color
branch
from
45e71e7
to
8706c2d
Compare
Base automatically changed from
update/fix-issue-on-color-picker-component-remove-tiny-color
to
trunk
October 15, 2021 12:54
jorgefilipecosta
force-pushed
the
fix/we-always-pass-different-props-on-each-change-to-picker-component
branch
3 times, most recently
from
bf962ef
to
67a2f32
Compare
noisysocks
added
[Feature] Component System
noisysocks
approved these changes
Oct 25, 2021
This was
linked to
issues
Oct 25, 2021
jorgefilipecosta
force-pushed
the
fix/we-always-pass-different-props-on-each-change-to-picker-component
branch
from
67a2f32
to
9b82788
Compare
jorgefilipecosta
changed the title
jorgefilipecosta
deleted the
fix/we-always-pass-different-props-on-each-change-to-picker-component
branch
This was referenced Oct 27, 2021
vcanales
pushed a commit
that referenced
this pull request
Oct 27, 2021
ciampo
reviewed
Nov 8, 2021
There was a problem hiding this comment.
Late to the (review) party — I was AFK for the past 2 weeks. While I'm catching up, I thought I'd leave some inline feedback.
Also, as a general rule of thumb, it'd be better to get some feedback from components-folks before merging PRs that involve components, especially when these components are not experimental, in order to make sure that we avoid introducing breaking changes as much as possible and we keep the docs up to date (cc @griffbrad)
| onChange, | ||
| defaultValue, | ||
| defaultValue = '#fff', |
There was a problem hiding this comment.
We should update the README and any potential JSDocs types to add the Default value of the defaultValue prop — @jorgefilipecosta would you be able to follow up with this small change?
There was a problem hiding this comment.
Sure I will work on a PR with this change.
There was a problem hiding this comment.
Hey @jorgefilipecosta , have you had any time yet to work on a PR with this change ? Thank you!
Comment on lines
-69
to
-73
| const [ color, setColor ] = useControlledValue( { | ||
| onChange, | ||
| value: colorProp, | ||
| defaultValue, | ||
| } ); |
There was a problem hiding this comment.
I'm not sure if moving away from useControlledValue caused any regressions — @diegohaz could you take a quick look?
There was a problem hiding this comment.
Hmm! I don't understand why we need to move away from useControlledValue in this case, but maybe this is an issue with the useControlledValue function itself.
I wonder if the fact that we're not memoizing the state updater here is causing any problem since the original updater from useState is supposed to be stable:
gutenberg/packages/components/src/utils/hooks/use-controlled-value.ts
Lines 35 to 38 in 8ccd5e1
There was a problem hiding this comment.
The only reason I removed the useControlledValue call was that everything seemed to be working as before without it. It seems on this case it is not needed.
There was a problem hiding this comment.
Just cc'ing @sarayourfriend who wrote this part of the ColorPicker component, just to make sure that removing useControlledValue was ok in this scenario
There was a problem hiding this comment.
The idea behind using useControlledValue was to allow the color picker to behave like a standard React input, that is, with or without a controlled external value.
<ColorPicker onChange={setColor} />Should work even though the color prop isn't being passed. Just like <input onChange={setValue} /> works.
There was a problem hiding this comment.
This makes sense, thank you Sara!
@jorgefilipecosta , would it possible to add the useControlledValue hook back to the component?
There was a problem hiding this comment.
Hey @jorgefilipecosta , just wondering if you've had any time to work on this, or if you plan to anytime soon? Thank you!
There was a problem hiding this comment.
@ciampo and @jorgefilipecosta, I was investigating an issue with the color picker and gradients (reported in #36899) and it looks like re-instating useControlledValue and the debounce call resolves the issue, so I've opened up a PR in #36941 if either or you get the chance to take a look. I'm not very familiar with the ColorPicker component, so it can probably use some 👀 from folks who've worked with it before 🙂
There was a problem hiding this comment.
Thank you @andrewserong , I'll definitely have a look
| @@ -15,12 +15,14 @@ interface PickerProps { | |||
| } | |||
|
|
|||
| export const Picker = ( { color, enableAlpha, onChange }: PickerProps ) => { | |||
| const Component = enableAlpha ? HslaColorPicker : HslColorPicker; | |||
| const hslColor = useMemo( () => color.toHsl(), [ color ] ); | |||
| const Component = enableAlpha | |||
There was a problem hiding this comment.
While I understand why we switched from an object-based to a string-based picker, I don't understand why we switched also from an HSL-based to an RBG-based picker — could you explain why?
There was a problem hiding this comment.
The format we use outside the component is hex code which maps directly to RGB just by converting hex to decimal. Since we are changing to a string-based picker why not use the RGB one when it maps more directly? What advantage do we get in using an HSL string-based picker?
There was a problem hiding this comment.
Ok, it looks like the only difference would be about the input string format — while the underlying ColorPicker would still be the same. I guess using a HSL picker made sense previously, when this component used HSL internally to represent color values — but with the current state of the component, this change should be fine :)
Thank you for explaining!
Merged
7 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes: #35640, #35677, #35675
The color picker was having some random movements during small dragging interaction as we can see on this video shared by @mtias
Screeny.Video.11.Oct.2021.at.15.07.57.mov
At first, I thought it could be related to debouce usage but when I removed the debounce usage the component started to crash. Because of recursive setState calls.
What was happening is that the react-colorful picker components expect that if there are no outside changes in the color the object passed to the component is the same as the component passed on the onChange.
In our usages we receive an object on the onChange then that color is serialized in the block attributes and we pass an object with the same color but the object may have some differences and is not the same object so the component thinks an outside change happened and recomputes the position of the picker that may be some pixels on the side (e.g: multiple pixels may be representing the same color and there is random movement between this pixels).
A simple way to resolve this issue is to use the string-based pickers so we always pass an equivalent string to the one we received on the onChange and the position is not recomputed and the component does not enter on recursive setState so we can remove the debounce.
It is not clear why the issue of the component when inside a popover closing during the first click is fixed. But I guess a combination of the debouce removal and fewer rerenders because of prop changes may explain it.
Testing
I replicated the steps shared on the video above and verified there are no random movements.
I used the component to choose colors by dragging the color picker fast and verified the UI was responsive so we can remove debounce usage safely.
I opened the color picker in a place where the color was not set before and rapidly clicked on the color area to select some color. I verified the custom color popover did not randomly close.