Borders: Fix border style on color/width clearing and global styles fallback logic #49738
Conversation
aaronrobertshaw
added
[Type] Bug
|
Size Change: +11 B (0%) Total Size: 1.37 MB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
Looks good to me. Thanks for the quick fix.
It addresses the bug reported in #49677 and preserves the original intention behind showing a default width and
| This branch | Trunk |
|---|---|
 |
 |
I just had one question in relation to allowing users to set a style (and nothing else).
I wasn't sure whether it was covered by the comment under "Next Steps" in the PR description.
Only apply the fallbacks for the Global Styles border panel as this prevents individual blocks from inheriting from a global border style
packages/block-editor/src/components/global-styles/border-panel.js
Outdated
Show resolved
Hide resolved
|
Flaky tests detected in 1dfb7dc. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4697353239
|
|
Thanks for the review @ramonjd 👍
When we were first adding the border support to global styles, it was suggested it didn't make sense really to set a global style only. So with what was under "Next Steps", moving the application of the fallback to the Global Styles' border panel, you can set only a border style for an individual block. In Global Styles though it aligns with the earlier feedback that you need to configure an entire border starting with width or color before style. |
aaronrobertshaw
changed the title
We need to set a border style when first selecting a width or color so that there is a visible border immediately. This change will ensure that if a user manually clears either the width or color field and there is no longer a configured width or color, that the border style is also cleared.
aaronrobertshaw
force-pushed
the
fix/clearing-border-width-color-fields
branch
from
1dfb7dc
to
4bbe9e5
Compare
|
I've rebased this onto the latest trunk to see if we can get the failing iOS e2e test to pass. |
There was a problem hiding this comment.
Just noting that this also suffers from the same issue that my PR had at the block level. In other words, if there's already a border style defined differently (using CSS for instance), it can be overridden. Granted that it's less impactful though.
Also, I think that we should always ask ourselves whether we're doing the right fix if the behavior between local (block inspector) and global (global styles) differ. In most cases it's not but I'm fine with this one for now. I just don't want it to create a precedent for us so we keep creating more of these edge cases.
|
👋 I tested this as per the instructions provided in the bug report this aims to fix #49677 I cannot reproduce this fixing the issue, I still see it happening. |
Thanks for flagging that @oandregal, it's odd. I've rebuilt this PR branch locally and it still tests well in terms of fixing the original issue in #49677. Here's what I see: Screen.Recording.2023-04-18.at.5.08.30.pm.mp4For me:
Can you confirm you are still seeing the original issue and if so perhaps share some extra replication steps or a video? |
|
Thanks for reviewing this PR @youknowriad 👍
I agree, that's a great question to ask and the guideline would work in most cases, just not this one until we update all the block inspector tools to use default values set from the merged global styles. +1 for not creating more edge cases, the nuances here have already proven how confusing they can be.
I still think we are on different pages regarding some of the issues around borders. I've retested this PR against #49714 and you can see the difference in the videos below. For testing purposes, I added the following theme.json snippet to the TwentyTwentyThree theme. ...
"styles": {
"blocks": {
"core/group": {
"border": {
"style": "dashed",
"color": "darkseagreen",
"width": "5px"
}
},
...
}
}
In this PR branch, you'll note that when editing only a color or width in the block inspector for an individual block, it honours the global styles' In #49714, you can see that setting a width or color only in the block inspector, will actually also set a When it comes to global styles, the value from theme.json is set as the default style for the border control. So in that instance, if there isn't a border style and we set a width or color, we do want a border style to be explicitly set. Hopefully, that is clearer now. |
What I'm saying is that global styles might not be the only place where one could apply a border style, custom CSS is another one for instance and forcing a style ("solid") that a user didn't chose has the potential to override that fallback. |
That is true, the Custom CSS comes with strong recommendations that it is supposed to be a last resort for things that aren't configurable via Global Styles. Of course, no recommendations will prevent the scenario where it isn't used appropriately or the theme loads its own stylesheet. Those scenarios would be much less common than a global style being set and someone tweaking an individual block's border. They were also deemed not worth catering for during initial feedback on the border block supports. So my comment about the desire for setting a border style when adjusting global styles was made from that perspective and to restore the border behaviour that existed prior to #48636. |
I must have done something wrong. I've tested again the block-level styles: it works beautifully now for me, and the width is no longer serialized to block. Global-level styles also works fine. 👍 |
Thanks for taking the time to re-test this one. Appreciate it. It's still working for me also so I'll get this merged. |
Fixes: #49677
What?
Why?
Without correcting the application of the fallback border style, moving it to the global styles panel, borders on the individual blocks accidentally override theme.json or global styles values.
Without the tweak to unset the border style when clearing a color or width individually, the user has to reset the control via the border panel's menu to get rid of the border style that was added as a fallback.
How?
Updates the change handling to apply an undefined border style when the width and color haven't been set. This application of a fallback will now only happen in the global styles border panel.
Testing Instructions
dashedborder styleScreenshots or screencast
Screen.Recording.2023-04-14.at.5.32.28.pm.mp4