Style Engine: Add support for text columns (column-count) #46566
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
Referring to the column count CSS support as text column was a deliberate decision during earlier iterations of the block support.
aaronrobertshaw
added
[Type] Enhancement
|
Size Change: +908 B (0%) Total Size: 1.32 MB
ℹ️ View Unchanged
|
andrewserong
approved these changes
Dec 15, 2022
There was a problem hiding this comment.
Nice one, glad to see text columns picked up again!
In terms of the introduction to the style engine and support for outputting this rule, this all looks good to me:
✅ Looks like it's added to the right places
✅ Tests pass
✅ The logic of calling it a different name in the data structure sounds good to me (textColumns) since it would be easy for column-count to be confused
In playing around with the setting in the linked PR, I noticed a peculiarity with how column count works with paragraphs. From a bit of Stack Overflow searching, it sounds like browsers might be adding some default -webkit-margin-before spacing or something of the like? The result is that there's a bit of spacing at the beginning of the first column that isn't present on the subsequent columns. It looks like adding a rule allows this to be removed. So, maybe not for this PR, but I wonder if when the rules are output, if it needs a preset (or preset-like) has-text-columns class added that resets things as they need to be?
| Before adding a reset rule | After adding a reset rule |
|---|---|
 |
 |
In terms of the style engine change, though, this looks good to me, so I've given it the ✅. Might be worth getting a second opinion from @ramonjd too, since I'll be AFK shortly.
|
LGTM Thanks for updating the tests! |
|
Thanks for the speedy reviews @andrewserong and @ramonjd 🚀
How this manifests itself, might be a little different based on where the column count style is applied. After you flagged the issue, I noticed that if I applied the I'll merge this PR as is, and we discuss things further in a follow-up |
Oh, good find! Yes, totally stuff to look into / explore separately 🙂 |
dmsnell
pushed a commit
that referenced
this pull request
Dec 15, 2022
ramonjd
added a commit
to ramonjd/wordpress-develop
that referenced
this pull request
Jun 21, 2023
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.
Related:
What?
Adds
column-countaka "text columns" to the style engine.Why?
The addition of
column-count("text-columns") to the style engine provides the base to add it as a block support as per #33587. Further history can be found via that PR and the following issues:How?
Testing Instructions
npm run test:unit -- --testPathPattern packages/style-engine/src/test/index.jsnpm run test:unit:php -- --filter WP_Style_Engine_Test