Columns: Add border support #31737
Columns: Add border support #31737
Conversation
aaronrobertshaw
added
[Type] Enhancement
|
Size Change: +5.15 kB (0%) Total Size: 1.07 MB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
This is looking good to me. Not necessarily a problem with the PR but I did find it slightly confusing that because the "Default" style is undefined, I could make changes to the color/width selections but not see them reflected until I selected a different option from the border style dropdown.
I confirmed:
- Border style, color, radius, and width work for the outer Columns in the editor
- All options work for the outer Columns on the frontend
- Only the right-hand border is visually updated for inner columns
- Setting border options for the last column have no visual effect, but take effect when that column is moved to a new position
- Border style, width, and radius work on the frontend for inner Columns
One small issue: it looks like border color does not reflect on the frontend for inner columns:
| Editor | Frontend |
|---|---|
 |
 |
|
Thanks for testing @stacimc!
Agreed. There is a PR aiming to refine the border support UI (#31585). It will help improve the experience a little. The issue you describe has been raised though no decision was made on the best path forward for the short term. Longer term though there is the idea to allow borders to be "added" rather than just tweaking border attributes. So when a border gets added it would include a default border style that is visible, alleviating this problem.
Looks like the named color class isn't applying correctly, I'll fix that. Nice catch 👍 |
|
The application of named color classes in the save function has been fixed (e67a8b6) |
There was a problem hiding this comment.
I don't have write access to approve, but LGTM! The inner column border colors are now applying on the frontend as well.
There is a PR aiming to refine the border support UI (#31585). It will help improve the experience a little. The issue you describe has been raised though no decision was made on the best path forward for the short term.
Agreed, that's definitely seems like a much bigger conversation. Thank you for the link!
One last tiny thing I noticed is that the inner columns have a 32px left margin but no right margin; with borders applied this becomes noticeable because the content does not appear perfectly centered between borders:
This isn't a problem with the border support application and could be considered a separate issue in my opinion, though.
Thanks @stacimc, I agree. My thoughts were in the PR description as follows:
@andrewserong is currently working on adding spacing support to the Columns block. My understanding is that work should be ready to land very close to the time this PR will get merged. |
|
This works as advertised for me, but I wonder if we want to add some additional css to handle the borders when stacked in mobile, eg. it currently looks odd with the border just on the right and not on last child?
|
Thanks @glendaviesnz, definitely a good call. I've added some responsive styles for the application of the column borders. I wasn't sure on the best balance between reasonable default behaviour or just omitting the border entirely. I've made it such that the right border doesn't display on even columns when in a medium viewport.
On mobile I've applied the border as a bottom border expect for the last column at the bottom of the vertical stack.
Ideally, the border support would be able to provide configurations for individual borders combined with responsive styles. Hopefully this is a reasonable option until then or the G2 project is brought into Gutenberg. Happy to roll back the bottom border change and just omit the borders entirely on mobile if that is deemed a better option. |
|
Just noting users/themers might want to have a border around the entire Column block and not just in between. What's shown in the video is redefining expectations around the current border controls. I see two options:
|
|
Thank you for the feedback @justintadlock 👍
Good point! I was focused on being able to offer something to allow block pattern designs with just a separating border. One unsatisfactory workaround could be to add a group block to the column and apply a full border.
I agree.
Unfortunately, at the moment that is the case across all block support provided styles however a solution to this responsive styling issue is being explored and worked on. In terms of possibly unlinking the border-width control and supporting individual side values, this might cause some issues with the currently proposed border support UI in #31337. Nothing insurmountable but worth noting. There are some other issues about whether to rely on the shorthand
I'm not sure how this would work exactly. It would not be a part of the border block support yet might need to implement similar control over the color, width, style etc. That overlaps a lot of what the border block support already provides. There would also be a problem relating to how this is added to the sidebar controls. This issue already highlights a range of design issues with adding more. I agree the current support doesn't fit the ideal 100% due to non-responsive styles and an inability to configure borders for separate sides. There might be an "Option 3". We could opt into the standard border support and allow for it to be used as per normal. The right only border could be achieved via a block style. That block style's CSS class could add a default right border, enforce only the right border is shown when appropriate, include media queries to switch it to the bottom for mobile etc. Given that covers a lot more bases, I'll make those changes. Thanks again for taking a look at this! |
|
I've updated this PR to use the new approach of a block style to achieve the divided style where only the right border is shown. This allows for the block support to be applied in a predictable manner. It also has the benefit of reducing the amount of code changed. The PR description and test instructions have been updated to reflect the changes. |
|
I like option 3. It's a good compromise between the first solution and a whole new divider control. |
|
The new approach looks good, the only issue I found was that the default grey border colour is barely visible on the likes of the tt1 theme:
something closer to black may be better. |
Fair point. I'd just picked that color as the |
aaronrobertshaw
force-pushed
the
add/column-border-support
branch
from
b59db37 to
8bd1947
Compare
|
I've rebased this PR after the recent merge of the refined border support UI in #31585. It would be great to get some further testing and feedback on how borders are applied to columns. @jasmussen Any thoughts? What might we need to improve for border controls on columns to be ok to unlock. |
|
This brief demo illustrates some of the rough edges present with the current experience. Screen.Recording.2021-07-09.at.4.25.12.pm.mp4 |
|
Took this for a spin:
While the end result here isn't super compelling visually, the tools present definitely show their power and what can be unlocked by adding border support. So overall, this seems on the right track. A few things became clear in testing this. First off, we really need a new color swatch soon, because the "Clear" button here feels like it should clear all the contents of the panel, not just that one particular color:
That's a problem specific with the color picker, and not with the border panel especially. It's nice that the "Divided" style lets you customize just the border between columns. That, specifically, feels like it can enable some compelling patterns. However this is made slightly trickier to accomplish by only working when the border is set on the column (singular), rather than the entire columns block itself:
Would it be possible to add border support only to the parent columns block, and not to the child column block as a starting point, and make the divided style work directly there? While selecting the individual column blocks have become easier, it's still a little fiddly. And it seems like if the border support is to be aware of the divided style, the border properties should also be shown in context of the same style picker. That would also, presumably, allow us to avoid the situation where you have to set the border properties for each column in order to accomplish the gridded look, i.e. avoid this:
|
| border: 1px solid $gray-900; | ||
|
|
||
| // Disable top and left borders. | ||
| border-top: none !important; |
There was a problem hiding this comment.
If at all possible, we should avoid !importants. If we need to increase specificity instead, that would be preferable. I could imagine a particular theme wanting to customize this style variation further.
One way to increase specificity would be &.is-style-divided.is-style-divided .wp-block-column. It's slightly hacky to duplicate the same classname to increase specificity, but it works, and should be preferable to !importants.
Another option is to reduce the specificity of whatever needs overriding. Lately I've become fond of using :where for that: https://codepen.io/joen/pen/abJVrGX
There was a problem hiding this comment.
If at all possible, we should avoid !importants. If we need to increase specificity instead, that would be preferable.
Allowing users to configure border colour, style or width via these block support controls means those styles will be applied via inline styles. To overcome those and disable the other sides to give this "Divided" block style !important is required.
The comment at the top of that CSS section defining "Divided" styles explains this. Does it need to be made clearer or more prominent?
|
Another thought: the divided style is very compelling for patterns. However by being a style variation, it's also tied specifically to this block, and to this style being present. Honestly the visual is compelling enough that we might want it to be a core part of the border support feature directly, rather than something that requires a style choice. Being able to style the outer and inner borders separately, I could see being compelling also inside Table or Media & Text blocks, possibly even Buttons or others. That does put some extra strain on the border UI to be able to accommodate that extra complexity, but it might be worth it. Is it okay to ship the feature that relies on the style variation in the mean time? Maybe, but I'd love thoughts by global styles savvy folks such as @nosolosw or @jorgefilipecosta if they have time (thank you). Overall, really nice work on this, thanks for all the effort you put into this. |
|
Hey, I took a look at this. Thanks for pushing what's possible to support cool designs, Aaron! I've tried it a few times. First, with excitement; then, growing slowly concerned 😓 My concern boils down to this: we're coupling the style variation of a parent block to the individual properties of the children. Examples: the use of The effect would be super cool to have, though. It'd be great to look at how to accomplish it via a standalone mechanism. If we can do it via block supports it'd be fantastic. In terms of subcomponents for this to work as a block support I'd think we need 1) being able to tweak side borders individually (as we do with margin or padding) 2) attach behavior to borders (enable right or bottom depending on some conditions, etc) and 3) allow the parent to absorb the inspector controls of the children so users don't need to go child by child to set the same conditions. I think I could explore 3 if nobody else has gotten to it yet. It's related to #33255 and would benefit many design tools. |
Agreed. This isn't ideal. I didn't see a means of achieving this without that or completely overhauling the border block support where it then gets further bogged down by theme.json and global styles support. From the perspective that all the individual columns belong to their parent, that relationship by definition means they'd be affected by the parent's styles. I wondered whether that would be sufficient to allow the parent's styles to dictate that certain sides of a border on an individual column wouldn't show.
This was the only means to override block support inline styles. The use of
The block style could be moved to the individual column level. That would mean the block style is all in one place in terms of blocks however it's now also multiplied by the number of columns. The same issue as the per column border configuration.
The block style has its own default CSS so I believe it would work even without borders enabled. The theme author can then also set their own styles in CSS. The block support side of it would only allow leveraging of theme.json, global styles and individual block configuration. Even that isn't clean but provides the most flexibility. There are a lot of designs out there for columns and it could be difficult to draw the line on which should be achievable.
Achieving this under-the-hood looks like it would be much easier than providing the UI to manage it. Most PRs along this one's lines will create discussion about a proposed "appearance" panel. To hopefully fast-forward that, I'll include the mock up below. One of the issues I have with it is it isn't clear how we can get to that stage. We'll also need a design around how to specify individual border sides. Would we only rely on splitting border widths? Or, do we wish to also allow different colors and border styles per side?
Could this also begin to extend into the realms of responsive block support values? Do you have any ideas on how this would work if we are only configuring this block support via block.json?
I'm not aware of anyone tackling that one yet. Let me know if I can assist at all. |
I'd favor this approach as well. Once the border tools are capable of handling specific edges well we can revisit the children, but right now having borders on the entire columns set can be quite useful. Can we get that in as a first step? |
Absolutely. This PR has been on the back-burner for quite a while but I can update it accordingly in the next few days.
It might also be of interest that there are new border control components approaching a point where they can land. Following that, there is a related PR extending the border block support to allow the configuration of individual side borders. Essentially, it might not be too long before we can have it all. |
|
Indeed, seems we can do a quick one just to add border to the container, continue the work on the panel itself, and extend the support to single column blocks once that's solid. |
This reverts commit 017a0f2173dba7b2a948a99816fa7e3f95cde2f2.
aaronrobertshaw
force-pushed
the
add/column-border-support
branch
from
a8c790d to
2dc39f2
Compare
|
I've updated this PR to only opt into the border support for the Columns block. This reduces the changes to a simple tweak of the block.json. While testing, I noticed that the available colors in the Global Styles border panel only show colors from the "last source" palette. I'll create a fix for that in a separate PR. |
|
Nice one. Before: After:
The branch is very old so it looks like it hasn't picked up recent changes to the border panel. Even if the checks are green, it might be worth doing that rebase just so there aren't any surprises that happened to the UI in the mean time. Otherwise, it seems like a good start. Thank you! |
@jasmussen I'm not sure what you are referring to here. This was rebased yesterday before pushing the changes. Am I correct that the issue is that the border controls for the columns block weren't made defaults? I've pushed a new commit adding this. |
There was a problem hiding this comment.
This is looking good to me @aaronrobertshaw, nice idea starting with the parent Columns block support for the moment.
✅ Works well at the individual block level
✅ Adding border in global styles for the Columns block works correctly
✅ Borders at the individual level override styles set at the global styles
LGTM!
| Editor | Global styles |
|---|---|
 |
 |
It's very likely the behavior of the border panel that was confusing me. It's been a bit back and forth on which controls get added at the same time or not. Thanks for landing this. |
|
Thanks for pushing this over the finish line |
In anticipation of #37770 landing soon a new PR has been created to explore adopting border support on the |
Related: #21889
Description
Previous PR Description (included block style approach to dividing borders for inner columns)
Description
Notes
Block patterns that may use this feature
Screenshots
How has this been tested?
Manually.
Setup
appearanceTools: trueunder settings.Test Instructions
Screenshots
Types of changes
New feature.
Checklist:
*.native.jsfiles for terms that need renaming or removal).