Block Supports: Add text columns (column count) to typography block supports #33587
Conversation
aaronrobertshaw
added
[Type] Enhancement
aaronrobertshaw
requested review from
ellatrix,
oandregal,
spacedmonkey and
TimothyBJacobs
as code owners
|
Size Change: +210 B (0%) Total Size: 1.34 MB
ℹ️ View Unchanged
|
|
This is looking great, and, in my view, satisfies the feature requests in #25459 and #27118, though I'll leave it to @scruffian and @iamtakashi to judge. I mostly tested with
I think it's good to go. The only question I had was whether we should clear the value in the UI when we reset the column count to make it clearer to the user that no column count is specified (and therefore may be overridden by global styles). Dragging the column count down to <p style="column-count:1">...</p>The consequence was that global styles didn't apply to this particular paragraph:
I would expect this to be the case. I think it makes sense to be able to specify that a block should always have Still, since hitting "Reset" changes the UI value to I could be overthinking it. |
|
Thanks for the review @ramonjd 🙇
That's a good question. The current behaviour comes from setting the initial position on the slider control. It might be better to omit that so the input can reflect the "unset" state indicating that it's currently inheriting from the theme or global styles. Originally, I was thinking along the lines of the columns control for the gallery block that was currently a slider which always displayed a value. There could also be a case to change this to a stepper field. In the short term, I'll try and improve its behaviour without switching controls. Then wait on further feedback regarding the UX.
I don't think so. It's definitely a rough edge that could do with smoothing out 🙂 |
|
The column count's When the new progressive disclosure panel lands and the typography block supports panel is updated to use it, we'll likely want to reduce the control's footprint. It also won't need the reset button. I'm starting to lean more towards changing this to a simple
|
Thank you!
Makes sense to me, particularly after having just taken that PR for a spin. Speaking for the reset button, it is more explicit in that you know what's going to happen when you click on it. As we continue to test the panel and controls like this one, maybe we can work out a neat way to communicate that values are reset/won't persist when toggling controls in the progressive disclosure panel.
Nice idea 👍 More so given the range is 1-5. I'd understand the need for range controls if we were dealing with 0-100 with step values. |
|
One challenge here is to not confuse users when it comes to Column blocks and this text columns setting. We'd need to be careful with how the UI controls are presented, and probably add further contextual text around what this does (cosmetically and not structurally, meaning a user won't be able to add other blocks to a column). For example, I'm not entirely sure this belongs in the typography panel. |
|
For example, maybe we can consider this more of a block style rather than a full on setting, and only support two columns. |
|
Thanks for the feedback @mtias. 👍 I'm happy to adjust this however needed to avoid any confusion with the existing Columns block. While creating this PR I had a couple of things in mind:
It appears I misunderstood what was initially being proposed regarding block styles. I was under the impression the style attributes might also need to be supported via theme.json and/or block supports. I don't have any strong opinions on where this should belong. As the discussion around this evolves I'll continue to make any requested updates. |
|
Yes, I think the result is valuable to have, we just need to explore a bit the UI controls and what makes the most sense. |
A "layout" panel would have more sense, in my opinion. Is there a plan to add more column styles like column-gap, column-rule, etc.?
I think that the fact that this setting offers fewer functionalities than the block could help users understand that this is the quick way to add columns, while the block is the tool to use if they need more control. |
I'm not certain here. A layout panel is already injected for certain blocks to control content width and full width values etc. If we are trying to keep the delineation clear between columns blocks and other more structural changes, it feels more typographic to me.
Some additional control over styles is on the horizon. #32571 adds the ability to control gap spacing and this would be included under the new "Dimensions" panel. |
|
As the group block is getting more flex-features, and is even growing closer to the columns block, it seems like there's an opportunity to share some DNA in the interface, perhaps even in the underlying code. After all, what is the major difference between a 1 column Columns block, and a Group block? (I know there are some exotic alignment based ones). The thing is, I share Matías concern about the confusion. Moreover, I have bouts of excitement about the built-in column-count features, then I implement them and remember the downsides:
Specifically, box-shadows and borders, anything that's on the outside of an element, those are broken across columns. There's little to no control of it, and you can even end up in situations where the preceeding element's bottom margin pushes down the next column so it doesn't start at the top. You can tinker with CSS to mitigate things with In other words, I find |
|
Thanks for highlighting the issues with CSS columns @jasmussen .
Having It comes down to whether there are enough blocks that would benefit from it, that also don't need borders or shadows, to warrant it being implemented in a shared way such as this PR does. We could still switch to offering it as an ad hoc solution for the post excerpt block etc. In that case, it would be good to offer a Also, for what it's worth, I still think typesetting is different than layout and so the CSS columns fit better in the typography panel than the layout one. Happy to be overruled there, it's a minor issue in the scheme of things. |
The tools menu could offer the choice, that's the benefit of it! Though per the feedback I think we should at the very least start with it not shown by default. |
|
@jasmussen I figured I should mention that the issue with borders/shadows being split across columns can be addressed via the |
|
Let's pick this one up and drive it to the end this time :) |
| @@ -65,6 +65,7 @@ class WP_Theme_JSON_Gutenberg { | |||
| 'padding' => null, | |||
| ), | |||
| 'typography' => array( | |||
| 'columnCount' => null, | |||
There was a problem hiding this comment.
Can we change the attribute to be textColumns instead?
There was a problem hiding this comment.
Well, technically, it's not just columns for text: something using this property could have images and other media in its content.
In fact, are we even sure this belongs in the category of typography tools? It technically has use cases outside of bare text, especially once you start using break-inside to preserve the integrity of child containers. A gallery could hypothetically use it to layout its images, for example, as could a series of listing of social media posts. I'd argue it actually belongs with the layout tools.
I think "content columns" is a more strictly accurate term than "text columns".
There was a problem hiding this comment.
Well, the original issues are about unlocking a specific effect on textual content. I'm not sure we should design it as a general tool and expose it as one were it'd start to compete with other layout tools. "Content columns" is just very close to regular columns, which are also about showing any content in columns. Here we need to represent the flow and continuity aspect, which seems better captured by "text columns" to me. It might be better placed in "layout" regardless, but with the right context.
There's also a question on which blocks this should be exposed. For example, wouldn't make any sense to have this on the headings block.
There was a problem hiding this comment.
The interesting thing about CSS columns versus typical divs-in-divs "columns" is that:
- Divs-in-divs columns have content explicitly assigned to a column, and the height of each column can vary wildly.
- CSS
columnshave content dynamically placed in the columns, often attempting to balance the height of each column (though a minimum content height for a column can be set).
These have big ramifications for when you would want to use one over the other, but I have no clue how to convey these subtle but important differences to users.
Here's the only blocks I can think of where this control would make sense:
- Buttons
- Gallery
- Group
- List
- Navigation
- Paragraph
- Quote
- Verse
In other words: most of the container blocks, and maybe a few standard text blocks for the sake of allowing users to avoid extra divs in a few cases. In most cases, though, you'd actually want to avoid using CSS columns on a single Paragraph, since that's less future-proof for adding additional text. So maybe it should only be the container blocks that have the control, to encourage best practices.
There was a problem hiding this comment.
Left a request to change the attribute and function names to textColumns instead of columnCount.
We could explore a different UI with a segmented control and icons for single column, two-columns, and three columns. I don't think we need to support more than that for text-columns.
|
Thank you for the renewed conversation and interest in this @mtias and @ZebulanStanphill. I'll dust this off, bring it up-to-date and rename functions/attributes etc, later this week as I'm AFK tomorrow. |
aaronrobertshaw
changed the base branch from
trunk
to
add/text-columns-to-style-engine
|
My assumption is that 1 means 1 text column, which is the default for any element. So I would assume when adding this control, 1 is the preset, and lowest value. |
That's my thinking.
As text columns is an optional control that needs to be toggled on from the typography panel's menu. It would be reset via that same menu to opt out of setting any specific column number. The field can also be cleared without resetting via the menu. |
|
Thank you for clearing that up. |
aaronrobertshaw
force-pushed
the
add/typography-columns-support
branch
from
67ec105
to
37a48d2
Compare
|
I've reworked this PR in line with the recently extracted typography panel #47356. Given the degree of change required, this PR will need a full review with particular attention on whether the UI control displays appropriately based on theme.json settings or individual block supports. |
|
Testing well for me. I think that after this PR the |
|
Thanks for the re-review ping, I haven't had a chance to test this properly yet, but just on this comment:
I think that's the case, now that the resolver isn't in the versioned directory, it looks like the |
|
Thanks @glendaviesnz and @andrewserong for catching the incorrect theme.json. I'd lost track of this during the multitude of rebases after significant changes to theme.json/global styles panels etc. I've moved the theme.json tweak into |
There was a problem hiding this comment.
Thanks for all the follow-up here @aaronrobertshaw, this is testing great for me!
✅ Works correctly at the individual block level for both static and dynamic blocks (e.g. post excerpt, paragraph blocks)
✅ Works in global styles / theme.json, and can be overridden at the individual block level
✅ Control doesn't show up where it shouldn't (e.g. root level text elements in global styles)
✅ Switching textColumns to false in theme.json still allows the text columns styles to be displayed in the block editor and site frontend for blocks that already have those values set (e.g. in patterns or existing post content when a user switches themes)
From a re-read, the code's looking solid to me. The only comments I added is that after the rebase now that the Typography panel is consolidated between the block editor and site editors, is that it looks like we might be able to remove the text-columns.js file? I couldn't see it being imported, since typography-panel.js now has the logic in there directly using NumberControl, but apologies if I've missed something there!
LGTM! ✨
|
Appreciate the speedy reviews, thank you 🚀
Nope, the opposite, I missed it along with the It was kept around for reference when I was rebasing after the typography panel extraction. I then dropped the ball in terms of removing it once it wasn't needed. With all the feedback addressed, I'll go ahead and merge this one now. |
No worries, happy to be another set of 👀, it's tricky to see these things sometimes when you've been with a PR for a while 😄. Nice work landing this feature! |
|
I'll backport the PHP files for this one 👍 |
Related:
What?
Adds a new block support feature to the typography toolset to allow control over CSS column count for blocks displaying multiline text.
Why?
To open up further design possibilities.
Patterns enabled by this
How?
column-countCSS property.Testing Instructions
npm run test:unit:php -- --filter WP_Theme_JSON_Gutenberg_Testnpm run test:unit packages/block-editor/src/hooks/test/style.jssettings.typography.textColumnstotruesupports.typography.textColumnsproperty totrue.Screenshots
Screen.Recording.2022-12-15.at.1.13.05.pm.mp4