Color Picker: Re-instate debounce and controlled value to fix issue with gradient picker #36941
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
andrewserong
added
[Type] Bug
|
Size Change: +90 B (0%) Total Size: 1.1 MB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
Thanks for putting this together @andrewserong 👍
I believe this also fixes #36066 so I've added it to the PR description.
✅ Unit tests pass
✅ Original issue could be replicated
✅ After applying this PR, no error is thrown
✅ Reinstated code matches what was removed
✅ No severe unexpected movements to color selection indicator (see comment below)
When testing that this doesn't re-introduce any issues #35670 was aimed at fixing, I did notice a really small jump to the color selection indicator sometimes while slowly dragging it. I don't believe its significant, it's definitely better than the picker crashing. It also could be tired eyes 👀.
Untitled.mp4
In general, this is looking pretty good to me. I'd be inclined to approve it however it would probably be best to wait upon Jorge's review.
jorgefilipecosta
approved these changes
Nov 29, 2021
There was a problem hiding this comment.
Previously we had a bug on our code and on component rerenders even if the color did not change we were passing a different object to the react colorful component, and that caused a similar crash to the one happening here and unexpected popover closes. That issue was solved and now we use a string version of react-colorful and we only pass a different string if, in fact, a color change happened. But as we can see it is still possible to crash the component. This crash seems internal to react-colorful (we can see on the stack trace that the crash in one of the component hooks) we are just passing a string to it, and I checked using the debugger we only pass a different string when the color changes so I think the ideal fix would be upstream in react-colorful as it seems the component has a bug. That said this seems to fix the issue for us so I think we should merge this PR.
This PR does not revert the fix on #35670, the fix there was not to make sure we don't pass a different value to react-colorful on rerenders. It just seemed that the fix would allow removing debouncing and that is not the case.
Thank you for fixing this issue @andrewserong.
| value: colorProp, | ||
| defaultValue, | ||
| } ); | ||
|
|
||
| // Use a safe default value for the color and remove the possibility of `undefined`. | ||
| const safeColordColor = useMemo( () => { | ||
| return color ? colord( color ) : colord( defaultValue ); |
There was a problem hiding this comment.
Given that useControlledValue, is already taking into account defaultValue, I guess we can simplify this to return colord( color ).
There was a problem hiding this comment.
Good suggestion, thanks! I've simplified the return statement here and moved the inline comment to above useControlledValue as that's where the default value logic is now happening.
ciampo
approved these changes
Nov 29, 2021
There was a problem hiding this comment.
Echoing @aaronrobertshaw and @jorgefilipecosta , code changes LGTM 🚀
Thank you so much for looking into this fix, @andrewserong !
PS: before merging, could you add a CHANGELOG entry for this PR (and maybe add also an entry for #35670, as explained here) ? Thank you!
|
Thanks for the reviews and feedback, everyone! I've added changelog entries, and updated the Readme while I was at it to include the I'll merge this in once the tests pass, but let me know if any other follow-ups are required and I can open up another PR. |
|
I didn't get a chance to merge this in today (we were debugging failing e2e test failures separately in #36982), but 🤞 we can merge this one in tomorrow. |
andrewserong
force-pushed
the
fix/color-picker-reinstate-debounce-and-controlled-value
branch
from
b0694bb
to
5f96fc8
Compare
andrewserong
deleted the
fix/color-picker-reinstate-debounce-and-controlled-value
branch
2 tasks
7 tasks
This was referenced Dec 2, 2021
noisysocks
removed
the
Backport to WP 6.7 Beta/RC
noisysocks
pushed a commit
that referenced
this pull request
Dec 6, 2021
…ith gradient picker (#36941) * Color Picker: Re-instate debounce and controlled value to fix issue with gradient picker * Add changelog, update readme, simplify safeColordColor return value
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: #36066
Description
Fixes part of #36899, possibly introduced in (or related to #35670)
When editing a gradient (e.g. attached to a background for a Group block), if you open up the color picker and click and drag quickly, prior to this PR, the picker throws an error (reported in #36899).
By reinstating the
debouncein the color picker, when used as a controlled component (as in the case of the gradient picker) we avoid recursively updating state and causing the block and picker to throw an error.How has this been tested?
Test that this doesn't re-introduce any issues that #35670 resolved.
Screenshots
Before
Kapture.2021-11-29.at.16.58.23.mp4
Kapture.2021-11-29.at.16.56.26.mp4
Types of changes
Bug fix (non-breaking change which fixes an issue)
Checklist:
*.native.jsfiles for terms that need renaming or removal).