Add preset for line-height and use it in the UI controls #23177
Conversation
oandregal
added
[Feature] Design Tools
oandregal
changed the title
|
Size Change: +313 B (0%) Total Size: 1.12 MB
ℹ️ View Unchanged
|
| { slug: 'full', name: __( 'Full Size' ) }, | ||
| ], | ||
|
|
||
| lineHeight: [ |
There was a problem hiding this comment.
If the theme doesn't provide any, which defaults do we want here?
There was a problem hiding this comment.
@jasmussen @kjellr @mapk this PR implements presets for line-height, as per #23111 I was wondering if you had any suggestions for line-height defaults.
There was a problem hiding this comment.
EDIT: I see what you mean by default. I would go with:
small: 1 - will be the height of the text. If you make it smaller, weird things can happen when wrapping or next to other text.
regular: 1.2 - nice
large: 1.5 or 1.6 work well.
Will users be able to set a custom value as well?
There was a problem hiding this comment.
Also, I would make sure those values are unitless if they aren't already: line-height: 1.2;
| const { keyCode } = event; | ||
|
|
||
| if ( keyCode === ZERO && ! isDefined ) { | ||
| if ( keyCode === ZERO ) { |
There was a problem hiding this comment.
What this code does is that when the user types a 0, the whole line-height value is reset to 0. This is a nice little detail! However, I also wonder: this is preventing the user from introducing any value that includes a 0, as it'll be reset. Examples of values a user is not able to introduce: "10", "1.05", etc. Unless there's something that I'm missing I presume this restriction should be removed.
There was a problem hiding this comment.
@ItsJonQ what do you think of this? can we remove this logic?
There was a problem hiding this comment.
I believe so!! We'll be able to remove the manual keyDown handling once we migrate to using <NumberControl />
There was a problem hiding this comment.
So I've removed the existing handleOnKeyDown logic in 98b1468 and this seems to work fine for me as it is. At least I don't know how to break it! This is what I tried: click the arrow up/down, keyboard interactions (both starting with an existing value and with no value).
oandregal
requested review from
TimothyBJacobs,
ellatrix,
spacedmonkey and
youknowriad
as code owners
youknowriad
left a comment
youknowriad
left a comment
There was a problem hiding this comment.
How does the preset gets applied? Inline style, class, CSS variables?
Should we use the last one (CSS vars) as it seems like the approach we want to ultimately adopt for all presets?
| * @return array Filtered editor settings. | ||
| */ | ||
| function gutenberg_add_line_height_presets( $settings ) { | ||
| list( $editor_line_height ) = (array) get_theme_support( 'editor-line-height' ); |
There was a problem hiding this comment.
do you think we should add new theme support or just rely on theme.json?
My opinion for the moment is that we should stop adding new theme supports and focus on making theme.json good enough.
There was a problem hiding this comment.
I like the sound of what you say, however, I worry about what we'll ship to WordPress 5.5 -- once there, it's difficult to change. For context, this is the full picture of style attributes:
| Style prop | Status | Opt-in / Opt-out | How it works | What presets it uses |
|---|---|---|---|---|
| Existing styles (font-size, color, gradient). | Released to core, stable. | Opt-out via disable-custom-{preset-name} |
Presets: classes / Custom: inline styles with raw values | Each style prop defines its own preset. |
| line-height | Plugin-only, stable. | Opt-out via disable-custom-line-height |
Presets & Custom: inline styles with raw values | None yet (this PR adds line-height presets). |
| padding | Plugin-only, experimental. | Opt-in via experimental-custom-spacing |
Presets & Custom: inline styles with raw values. | None yet. WIP at #23176 |
| units (for padding) | Plugin-only, experimental. | Opt-in via experimental-custom-units |
N/A | N/A |
| link color | Plugin-only, experimental. | Opt-in via experimental-link-color |
Presets: inline styles with css variables / Custom: inline styles with raw values | Uses color's preset. |
I'd be open to not adding presets for padding via the conventional mechanism (add_theme_support) because it's experimental, but, unfortunately, I think we need to add them for line-height so themes can add their own.
There was a problem hiding this comment.
Also for context! I only have a bit more than a week of work before 5.5 beta and my goals are: 1) adding presets for line-height & padding, 2) consolidating the way style attributes are stored in the markup, 3) prepare core patches.
For 2, I wanted to engage you and @ItsJonQ in a convo after this PR. The link color mechanism (always inline styles: CSS vars for presets, raw values for custom) works for how we use it today, and gives us a nice method to tweak the values. However, I'm concerned about how that would work for the color status (visited, active, hover). I don't see how we can implement them using inline styles (I had this convo with Jorge as well).
There was a problem hiding this comment.
efe4c26 exposes the line-height preset as CSS variables like the rest of preset categories do.
ItsJonQ
left a comment
ItsJonQ
left a comment
There was a problem hiding this comment.
🎉 @nosolosw Thanks for working on this! It looks good to me :)
I think a good (near) future enhancement would be to replace the <TextControl /> with <NumberControl />. NumberControl should take care of a lot of intricate interactions as well as value parsing/handling. Some of the logic that was refactored in this PR (that you pointed out) attempted to do some of these things with <TextControl />.
Maybe we can do this with the FontSize controls as well :)
(For the future! For now, this PR is 👍 !)
oandregal
added
the
Global Styles
This reverts commit cf2685f. See conversation at #23177 (comment)
oandregal
force-pushed
the
add/preset-line-height
branch
from
98b1468 to
ddf1c2f
Compare
This reverts commit cf2685f. See conversation at #23177 (comment)
oandregal
force-pushed
the
add/preset-line-height
branch
from
ddf1c2f to
735ff9b
Compare
| } | ||
| ], | ||
| "line-height": [ | ||
| { "slug": "small", "value": 0.8 }, |
There was a problem hiding this comment.
Labels are missing, it will become an issue sooner than later.
|
Thank you for your work, this is really cool. In looking at this, I'm reminded that the design tools are going to multiply many-fold and eventually take up a great deal of space in the sidebar. Even with just two controls, we're up to a lot:
I've worked a bit with Q on some mockups for design tool improvements, that emphasize compactness so as to actually fit multiple:
Here's a mockup showing only color and spacing controls in the sidebar, and I've done my best to make it compact — and even here, it takes up a ton of space:
While we can probably ship with the preset as is, as it mimics the font size field, we should start to think about what we can do to compactify these things. Common to all the design tools I've sketched for Q so far, is that they all take up exactly 36px height in a horizontal line, or 72px if they are "big", like the expanded spacing controls. I don't love the following, but it's an idea that emphasizes the same collapsing that happens to the color swatches:
In most editors, I'd seek out a much more traditional layout for these tools, something like ... but the combination of these being mostly theme-controlled presets with the custom input being optional makes it more related to Figma style presets, which has helped inspire these. I would love your thoughts on this as a general pattern, before we move too fast. And nice work Andrés! |
Q and I talked yesterday about the padding PR and accomodating presets over there also requires some thought to make it compact. Given that context and the other engineering issues that adding presets require us to do (thread), this is an area best explored with no rush. I'm going to close the PR to make sure it doesn't get accidentally merged and changing my focus for a bit. We can always reopen if this is a useful first step after 5.5. Please, be welcome to continue the design convo here if it's a useful venue! |
|
I don't think presets like "small, medium, large" work very well either for most of these controls. |
Implements #23111
This PR adds a new preset for line-height that themes can use similar to how they use colors, font-sizes, and gradients. It also adds the ability to reset user-defined values.
UI footprint
Interaction
Test
Follow-ups