Heading: Add line height support #21215
Merged
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
ItsJonQ
added
[Type] Enhancement
ItsJonQ
requested review from
ajitbohra,
jorgefilipecosta,
Soean and
talldan
as code owners
youknowriad
reviewed
Mar 27, 2020
| h4, | ||
| h5, | ||
| h6 { | ||
| line-height: var(--wp--typography--line-height); |
There was a problem hiding this comment.
Should we move this here instead https://github.com/WordPress/gutenberg/pull/21215/files#diff-f2f5efb13e0e16e61312aa9b0c028ae1R8
There was a problem hiding this comment.
@youknowriad The link doesn't appear to be working for me 😊
There was a problem hiding this comment.
a few lines up in the same file (with the other variables) :)
There was a problem hiding this comment.
Ah. Something like this?
:root {
h1,
h2,
h3,
h4,
h5,
h6 {
line-height: var(--wp--typography--line-height);
}
}
h1,
h2,
h3,
h4,
h5,
h6 {
background-color: var(--wp--color--background);
color: var(--wp--color--text);
&.has-background {
padding: $block-bg-padding--v $block-bg-padding--h;
}
}
There was a problem hiding this comment.
No, I think I missed the :root thing when I merged the heading PR. I was thing more like this:
:root {
h1,
h2,
h3,
h4,
h5,
h6 {
background-color: var(--wp--color--background);
color: var(--wp--color--text);
line-height: var(--wp--typography--line-height);
}
}
h1,
h2,
h3,
h4,
h5,
h6 {
&.has-background {
padding: $block-bg-padding--v $block-bg-padding--h;
}
}
There was a problem hiding this comment.
Ah gotcha! Thank you for clarifying :). I'll make those updates
|
Size Change: +124 B (0%) Total Size: 857 kB
ℹ️ View Unchanged
|
|
just want to confirm with the design team whether we want that feature in heading otherwise it LGTM |
youknowriad
approved these changes
Mar 27, 2020
|
Heading line height is a very good idea! It will create some nicer effects bringing in more importance to the header. Giving an option to the user to make the header stand out even better. |
|
Yes yes. Please include the line height on the Heading block. 👍 |
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.
Description
This update adds
line-heightstyle support to the Heading block!How has this been tested?
Types of changes
supports: __experimentalLineHeightThis update adds
__experimentalLineHeightto theHeadingblock settings. This newsupportsattribute was defined when introduced line height to the Paragraph block.By adding this flag, the
LineHeightControland attribute is automatically added to the Editor, and the styles (CSS variables) are automatically rendered with the new internal hook.CSS
In order to render this change, we need to add CSS for the Heading block. We're leveraging the
:roottechnique (renders::root h1, h2, h3, etc...) that bumps the specificity in a relatively hands-off manner. There's discussion around this technique here: #21037Note: Depending on how a theme's CSS is coded, these new line-height styles may not render correctly (due to specificity)
Checklist:
Along with #20775, this PR resolves:
#20339