BoxControl: Prevent invalid style values #33444
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
aaronrobertshaw
added
[Type] Bug
|
Size Change: +341 B (0%) Total Size: 1.08 MB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
Thanks for working on this @aaronrobertshaw and for rolling out the fix to the axial control, too (and for all the helpful inline comments).
I noticed that #33319 is resolved when I clear the value from the unlinked side and then commit it via the ENTER key, but not if I tab or click out of the field:
Other than that, these changes fix the issues with empty units appearing in markup for me, and the other testing steps worked well 👍. And I like the choice to use state for tracking the selected units so we can support the empty values.
Happy to do more testing tomorrow!
|
Your reviews @andrewserong are always great and much appreciated 👍
I did consider this. The existing behaviour was that the original value was retained after a blur event such as tabbing or clicking out of the field. If the field was cleared then committed via the It felt that the existing behaviour on blur could be considered an abandonment of editing the field. Submitting an empty value and having that result in a I'm more than happy to alter this to set the empty value on blur as well if others agree. There is also the
I couldn't see many other options to be able to maintain the current UX. The downside was this did introduce a range of edge cases resulting in a much larger PR. |
Thanks @aaronrobertshaw, I agree with your thinking there — better to fix the issue that's clearly not working correctly than make too many assumptions and add too much extra complexity. In my testing, I actually didn't mind the blur behaviour retaining the value, and needing to hit ENTER to commit it.
Ah, yes, thanks for the reminder — good to know it'll be a fairly straightforward change if we do want to add it in. Because the existing behaviour is that blur doesn't commit the value, I don't think this discussion should block this PR from moving forward, we can always address it in a follow-up 🙂 |
Thanks for working through the issue with me @andrewserong. After some further discussion and thinking, I'll split the changes to the NumberControl out into a separate PR so this one is solely around the BoxControl, its unit selection management and not saving invalid styles. The new PR can continue the discussion on the NumberControl and seek some fresh ideas or opinions. I don't believe it's required by the BoxControl changes. I'll link it back here for posterity. After that's done we should be close to merging this. |
aaronrobertshaw
force-pushed
the
fix/box-control-saving-invalid-values
branch
from
c9c2245 to
950d59f
Compare
|
I've split out the |
This was referenced Jul 16, 2021
This also maintains CSS unit selections between the "all" control and the unlinked controls.
This will be covered by the new test in the NumberControl PR fixing that bug. Once both PRs land a new one can be created updating the use of the BoxControl's unit and number controls adding required tests then.
aaronrobertshaw
force-pushed
the
fix/box-control-saving-invalid-values
branch
from
ca2fe1c to
b84ecb0
Compare
|
Now that #33485 has been merged, I've rebased this to pick up those changes.
It's worth noting the updates for the @andrewserong any chance you could give this a final sanity check and approval? While it is a bit of an edge case I'd still like to land this fix before it causes anyone further issues. |
andrewserong
approved these changes
Jul 27, 2021
There was a problem hiding this comment.
Thanks for the ping @aaronrobertshaw, this is working nicely for me after the rebase, no stray "CSS unit only" values, and the Enter key behaviour to commit empty values that we get for free, feels natural to me 👍
This was referenced Jul 12, 2022
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: #33318
Description
This PR fixes invalid values being saved via the
BoxControlfor block supports such as padding and margin.Changes made in this PR include:
BoxControlto manage unit selections via internal state so they can be separated from the block attributesonChangecallback to not save invalid styles meant updating how mixed values etc were determinedKey aspects of this PR
onChangehandlers broke the existing method of determining mixed values in the box control.Steps to replicate original issue
How has this been tested?
Via written tests and manual testing in block editor and Gutenberg Storybook
npm run test-unit:watch packages/components/src/box-control/test/Test instructions
Screenshots
Types of changes
Bug fix
Checklist:
*.native.jsfiles for terms that need renaming or removal).