Add a debounce to the custom color onChange event #40285
Merged
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
glendaviesnz
added
[Type] Bug
|
Size Change: +120 B (0%) Total Size: 1.22 MB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
Nice one @glendaviesnz, I think adding a debounce makes good sense here.
It nicely improves the performance of typing in the custom color label:
One challenge with adding in a debounce is that we then no longer have completely realtime feedback when updating the color and the custom color is in use (e.g. below I've set the background of the header's group block to the custom color):
On balance, though, I think it's better to free up the UI by including the debounce rather than have the performance penalty of the realtime behaviour. Perhaps to find a middle ground, would it work to reduce the delay to 100ms, or was 200ms already the sweet spot?
…d change timeout to 100ms
Have switched to 100ms - I think that is good enough for this instance as people aren't going to be typing a paragraph into that box. |
andrewserong
approved these changes
Apr 13, 2022
There was a problem hiding this comment.
Thanks @glendaviesnz, that feels like a good balance in my testing now 👍
I just noticed another issue, which doesn't appear to be related to this PR (I can also replicate it on trunk). If you go to update the label of the custom color, and then change the color, there's a seemingly odd situation where if you're updating the label, then changes to the color don't immediately flow through. This appears to be because updating the label changes the name of the CSS variable, so any blocks that reference the custom color now no longer reference it. As a user, I imagine the expectation in the UI here could be that references to the custom color would be updated at the same time, but I imagine implementing a feature like that could be super complex. Just thought I'd mention it in case anyone bumps into that while testing out this change (and flagging that it's unrelated to the addition of the debounce):
This LGTM! 🎉
|
@jorgefilipecosta Let me know if you can see any issues with this approach, otherwise I will merge this tomorrow. |
Provided things are working, I'm fine with this approach 👍 |
adamziel
added
the
Backport to WP 6.7 Beta/RC
adamziel
pushed a commit
that referenced
this pull request
Apr 25, 2022
Co-authored-by: Glen Davies <[email protected]>
|
This was backported to wp/6.0 branch to be included in WordPress 6.0 Beta 3 tomorrow: f2b9ad4 |
adamziel
removed
the
Backport to WP 6.7 Beta/RC
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.
What?
Adds a debounce wrapper to the onChange event for the custom color Option onChange prop.
Why?
Because updating the text of the custom color causes an update of custom colors array, each keystroke is causing a rerender of the full list.
Fixes: #38833
How?
Debounces the onChange method by 200ms to reduce the number of rerenders.
There may be a smarter way to do this, but because the index of the current item also needs to be passed into the callback I couldn't see any alternative to the current lambda that is being used for the onChange prop.
Testing Instructions