Block Support: Set border style none when border width zero #32080
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] Enhancement
|
Size Change: +160 B (0%) Total Size: 1.07 MB
ℹ️ View Unchanged
|
ramonjd
reviewed
May 24, 2021
|
I've updated this to also clear the border color selection and restore it if available when switching from zero to non-zero border width. The same as occurs for the border style. |
Nice! Retested and can confirm:
|
aaronrobertshaw
force-pushed
the
update/border-support-for-custom-units
branch
from
e4b68e8 to
41acd39
Compare
aaronrobertshaw
requested review from
ajitbohra,
spacedmonkey and
TimothyBJacobs
as code owners
7 tasks
aaronrobertshaw
force-pushed
the
update/border-style-when-width-zero
branch
from
1c1dafe to
e2703b0
Compare
aaronrobertshaw
force-pushed
the
update/border-style-when-width-zero
branch
from
e2703b0 to
0765a22
Compare
ramonjd
approved these changes
Jul 9, 2021
There was a problem hiding this comment.
LGTM
- Border styles persist in the editor when adjusting width from
0tonand back again (not between refreshes or switching between code and visual editors) - Frontend styles correspond to saved editor state.
Expand the border controls panel and select the solid border style option
Adjust the border width to a non-zero value, the solid border style button should remain on.
✅
Set the border width to 0. The border style buttons should all be deselected.
✅
Adjust the border width to a non-zero value again and the solid border style button should be reselected.
✅
Update the border width to 0 again, switch to the code editor view and confirm that the border style attribute is now set to none and the appropriate style is applied within the markup.
✅
Change the border style selection to "Dashed" or "Dotted" and repeat the process adjusting the width value.
✅
Confirm the block displays correctly on the frontend after saving.
✅
border-review-group.mp4
andrewserong
approved these changes
Jul 9, 2021
There was a problem hiding this comment.
This is testing nicely for me @aaronrobertshaw!
✅ Setting the width to 0 updates the inline style in the markup to set border-style: none;
✅ While adjusting the width in the UI, my previous settings are remembered. I noticed that if I go to the code view to check my markup and then switch back, my previous border style isn't remembered (since we're just using local React state rather than storing in the block's attributes). This seems fine to me — if I've switched out of editing the border, I don't expect it to remember my previous settings.
✅ The border style buttons render selected / vs deselected appropriately
✅ Works correctly with the dotted and dashed styles
✅ The block renders correctly on the front end
One other thing I noticed, which is separate to setting the border width to zero, is that if we have a non-zero value for border width, and de-select the border style, the border-width property still gets included in the inline style. Should that be omitted in that case?
I think that's unrelated to this PR, so I don't think it's a blocker for moving this one forward.
|
Thank you for the reviews @ramonjd and @andrewserong.
With no border style selected it would be inheriting from the theme.json. In that case the adjustment of only the border width would still be valid. So in this instance I believe it should remain. |
Ah, great explanation. Thanks for the reminder! That sounds good to me, then. 👍 |
aaronrobertshaw
force-pushed
the
update/border-style-when-width-zero
branch
from
0765a22 to
9f4f238
Compare
7 tasks
This application of the border-style: none will only occur if border style support has been opted into for the block.
This clear border color selection when the border width is explicitly set to zero. When it is set back to a non-zero value (including undefined) it will attempt to restore the previous color selection if available.
aaronrobertshaw
force-pushed
the
update/border-style-when-width-zero
branch
from
9f4f238 to
955250b
Compare
12 tasks
7 tasks
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.
Depends on: #31483Related issues: #31333, #31337
Related PRs: #31585
Description
As part of refining the border support provided UI, the border style dropdown is being replaced with a segmented control that does not include a
noneoption. The idea is that setting a border width of0should apply theborder-style: noneproperty.This PR updates the border width block support to update the border style attribute to
nonewhen the width is0. It then also re-applies the previous style selection if possible when the width is changed to a non-zero number, including being reset to undefined.How has this been tested?
Test Setup
Turn on custom borders within your theme.json file.
Test Instructions
0. The border style buttons should all be deselected.0again, switch to the code editor view and confirm that the border style attribute is now set tononeand the appropriate style is applied within the markup.Screenshots
ZeroBorderWidthSetsStyleNone.mp4
Types of changes
New feature
Checklist:
*.native.jsfiles for terms that need renaming or removal).