Add Line Indent support. #73114
Add Line Indent support. #73114
Conversation
mtias
added
[Status] In Progress
mtias
added
the
Global Styles
mtias
added
the
[Feature] Design Tools
|
Warning: Type of PR label mismatch To merge this PR, it requires exactly 1 label indicating the type of PR. Other labels are optional and not being checked here.
Read more about Type labels in Gutenberg. Don't worry if you don't have the required permissions to add labels; the PR reviewer should be able to help with the task. |
|
Size Change: +1.01 kB (+0.04%) Total Size: 2.57 MB
ℹ️ View Unchanged
|
|
Neat PR! This is cool to see. In testing this, I found two issues:
Here's a video showing both of these, including when it works automatically and when it suddenly doesn't: line.indent.mov |
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the Unlinked AccountsThe following contributors have not linked their GitHub and WordPress.org accounts: @ericmacknight. Contributors, please read how to link your accounts to ensure your work is properly credited in WordPress releases. If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
aaronrobertshaw
force-pushed
the
add/line-indent-support
branch
from
9839b49 to
53806f6
Compare
aaronrobertshaw
left a comment
aaronrobertshaw
left a comment
There was a problem hiding this comment.
Thanks for putting this PR together. It's always nice to see long awaited block supports becoming available 👍
For the most part this has been testing pretty well for me.
✅ Global Styles were applied as expected for both text element and paragraph block
✅ Block instance styles overrode Global Styles as expected
✅ TextIndentControl worked well (after quick tweak below)
I hope you don't mind but I took the liberty to push a few minor tweaks:
- Set an initial position for the TextIndentControl slider to match the unset state better
- Made text indent display by default in Global Styles panel as all controls are supposed to be shown there (at least this has been the long standing position)
- Noted the new block support in
global-styles-and-settings.md. More docs will need updating if we land this as a stabilized block support - Addressed linting errors
- I also gave this a quick rebase to avoid a build error
Before I dive back in to this for a final review, there are a few questions I'd like to float:
- Should we land this as a fully stabilized block support?
- For recent block support additions, the thinking has been to avoid
__experimentalsupports due to difficulties in cleanly deprecating these. - There's already an issue to stabilize all typography supports. A PR had even been merged before being reverted towards this stabilization.
- If we stabilize this new support before landing the feature, we will also need to update some other docs e.g.
style-versions.md&block-supports.mdetc.
- For recent block support additions, the thinking has been to avoid
- As the TextIndentControl is displayed as full width within the typography panel, it's order seems clunky.
- It makes sense that the Line Indent field follows the Line Height field but this means the flow between single column and full width controls is off. See below.
-
- Perhaps the Line Height and Line Indent could be rearranged after the text decoration/letter case type fields?
- It makes sense that the Line Indent field follows the Line Height field but this means the flow between single column and full width controls is off. See below.
- Is there a plan to make this support use presets?
- The proposed width/height block supports have been flagged as potentially benefitting from using presets, the line indent support could also reuse the spacing presets
- If so, all these new supports and the current ones for spacing and border radius, could benefit from a single shared component to render a preset slider and custom value toggle + input.
I'm happy to help out further here if I can. If not, I'll circle back and review again with fresh eyes tomorrow.
Of course not :)
Yeah, I think that'd be fine.
This could work
I think it could make sense as a follow up, yes. |
aaronrobertshaw
force-pushed
the
add/line-indent-support
branch
from
53806f6 to
d17f895
Compare
|
I've given this a rebase to fix the conflicts. I've also:
That works for me. Edit: Issue for using presets for line indent support can be found here: #73604 |
|
This PR should be good for some further testing. I'm out of time on this today but will sort out the backport PR tonight or Monday, then address any feedback across both. |
|
This is looking pretty good, thanks for the tweaks! |
aaronrobertshaw
force-pushed
the
add/line-indent-support
branch
from
80c90c2 to
5c5b271
Compare
aaronrobertshaw
force-pushed
the
add/line-indent-support
branch
from
5c5b271 to
54892f3
Compare
|
This PR should be close but the failing e2e looks suspicious. Locally, I'm getting the same error on trunk as this branch but it seems only this PR is failing on GitHub. The test is attempting to locate elements via the I'm probably missing something simple so will look with fresher eyes tomorrow. |
|
I've rebased this to see if that will get the unrelated e2e test failures to pass. If there are no objections, I'll get this merged early next week once they are all green |
|
I am an end user who primarily uses Japanese. Can we have an option to choose whether to skip the first paragraph or not?
In Japanese, indentation is normally applied to all paragraphs. We do not skip the first paragraph. https://www.w3.org/TR/jlreq/#line_head_indent_at_the_beginning_of_paragraphs Is the indent width specified in px units? In Japanese, the indent width is normally "1 em". https://www.w3.org/TR/jlreq/#line_head_indent_at_the_beginning_of_paragraphs According to #12451, in Chinese it is "2 em". I think it would be more convenient to specify the indent width in em units. This would also make it easier to accommodate cases where the paragraph font size has been changed. |
|
Thanks for the questions @mako09 👍
This might be possible but there could be some complexities in conditionally using different selectors in Global Styles based on a preference or selected option. We could also explore whether there are some more complex selectors available that would correctly apply the line indent style depending on current locale. If you have the time to create an issue that details the flagged use cases in more detail, that would really help to ensure we get the best solution possible.
The Line Indent control uses the 
Additionally, when defining the Line Indent value within theme.json you can also set the value using your preferred unit. |
|
Would it be difficult to replace "after the first one" in the description "indents the first line of each paragraph after the first one." with selectable options such as "excluding the first one" and "including the first one" to control this with an option? |
|
Thanks for the continued discussion here @mako09
As noted in my last reply, Global Styles support and generating different selectors based off of a selected option may not be feasible. At least not with the way theme.json styles are processed currently. I'm also not convinced that line indent support is a sufficient use case to justify another fork within the flow or generation of global styles. A better option might be for the line indent support feature to settle on a CSS selector that includes a means of targeting RTL languages that should have the first line indented as well. |
|
Personally, I would suggest reverting this PR before it's released as part of Gutenberg 22.4. Quote from this comment:
This is a quote from State of the Word, but Japanese is the second most used language after English at 5.82%. I don't think this is a problem that can be ignored: https://youtu.be/U_DF4-23C8Q?si=NZdkJFP2E5ye7vfX&t=462 I hope we could redesign it so that "include first paragraph or not" is a configurable option. |
|
Was line indentation ever meant to be an opinionated default, or should it be configurable? I ask because beyond Japanese, this affects Chinese, Korean, and potentially other writing systems. Rather than reverting, I'm wondering if we scope this feature more narrowly. Example:
I can also imagine sites that are meant to be translated in both |
|
This is another issue from the indent of the first paragraph. I am an end user without a development environment, and I have not tested the latest version. I am writing this based only on the information here and in #74200. In the current implementation, is the indentation achieved with a simple If so, I would be concerned about the following cases: I apologize if my speculation is off the mark. |
Seems similar to @t-hamano's comment over in #74200 (comment) |
|
Perhaps it would be better to have future discussions in #74200 to keep the discussion in one place. |
Unlinked contributors: ericmacknight. Co-authored-by: mtias <matveb@git.wordpress.org> Co-authored-by: aaronrobertshaw <aaronrobertshaw@git.wordpress.org> Co-authored-by: ramonjd <ramonopoly@git.wordpress.org> Co-authored-by: annezazu <annezazu@git.wordpress.org> Co-authored-by: skorasaurus <skorasaurus@git.wordpress.org> Co-authored-by: justintadlock <greenshady@git.wordpress.org> Co-authored-by: kjellr <kjellr@git.wordpress.org>
|
For those that might still be following along for this feature, there's a draft PR up in #74863 that explores one tentative approach to allowing arbitrary application of line indent global style to either subsequent or all paragraphs. |
Closes #37462.
Introduces a new typography tool to apply first-line indent to paragraphs. It can be applied locally or globally through GS. In global styles it works by skipping the first paragraph and indenting every subsequent paragraph instead. It resets across containers (like quotes).