Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update Line-Height Settings for opt-in (rather than opt-out) #23904

Merged
merged 1 commit into from
Jul 15, 2020
Merged

Update Line-Height Settings for opt-in (rather than opt-out) #23904

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion lib/client-assets.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -655,7 +655,7 @@ function gutenberg_extend_settings_block_patterns( $settings ) {
* @return array Filtered editor settings.
*/
function gutenberg_extend_settings_custom_line_height( $settings ) {
$settings['__experimentalDisableCustomLineHeight'] = get_theme_support( 'disable-custom-line-height' );
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since it was publicly discussed that line-height is shipping in 5.5. I don't think we should be reverting to an experimental theme support flag.

While ideally we'd focus on theme.json for the future. We can use theme support flags for padding and line height for this release.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As pointed out by @afercia in this comment, it's probably a bad idea to be exposing line height controls separately from font size controls in the first place, so I think it definitely should be made experimental so our options for improving it in the future are more open.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that's great feedback and we should improve on that. I consider these enhancements though and I don't think it's a valid reason to revert to the experimental stage. It's been used by folks in the plugin for a long time now.

Copy link
Member

@ZebulanStanphill ZebulanStanphill Jul 14, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So far, the only usage of the line height control I've seen has been very questionable usage. A lot of block patterns use custom line heights for headings and such, when this is almost never something you should do on an actual website. Your line height system ought to be consistent and set from the site-wide level. It's really weird to me that a lot of core block patterns use custom line heights (and font sizes), when these will almost always conflict with the user's theme. Core block patterns ought to adapt to any theme... otherwise they're only useful as tech demos.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we're powering a third of the web, we ought to stop promoting anti-patterns like one-off line heights and visual-hierarchy-breaking font-sizes for headings.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is actually already used in different block patterns. Even before FSE and Global Styles, the design possibilities for block patterns were very limited because of the lack of these controls, And sometimes they'd lead to developers/designers using non-semantic blocks to achieve the desired results (using headings instead of paragraphs, paragraphs instead of quotes...). The more control we'd give to blocks, the better block patterns we'd be able to produce.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the designers are using Paragraph blocks instead of Quotes, perhaps that's just because the Quote block needs to have the same styling controls as a Paragraph. (And also, the Quote block should probably use nested blocks anyway.)

Even back when some patterns were using Heading blocks when they should have been using Paragraph blocks, the Paragraph block already had font size options... even default font size presets provided by core! So I don't see how that's a good example.

Can you name any actual cases where a custom line height was required? I imagine it would have been a lot simpler if the core font size presets also changed the line height simultaneously, rather than providing a separate line height control.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both the current large-header and large-header-paragraph block patterns shipping in Core have custom line height without considering the third-party patterns that exist in the wild.

Your arguments suggest to me that you're arguing against the feature entirely and not the fact of it being experimental. I hope I explained above why this feature is needed but if you're still not convinced, feel free to open an issue to discuss it more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm... you're right. I guess I am arguing against the feature entirely. I'd rather see line height be explicitly tied to font size global preset controls. I want the only place where a line height control is shown to be the place where you define the font size presets in the Global Styles UI. I see zero reason to provide one-off line height controls; I think that allowing the user to define global defaults and their own set of presets to choose from a drop-down later on is the way to go. Because of this, I think providing a line height control is very premature.

However, that is not directly related to this PR, and with that in mind, I'm okay with this merging with the line height control remaining "stable", since making it opt-in is still an improvement over master and so we ought to get this in sooner rather than later.

(If I have the time, I may make some more PRs in the future to remove custom line height usage from core block patterns to make them fit in better with themes.)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's worth noting there are several issues already open related to this discussion including #23111, #23893, and #23895.

$settings['__experimentalEnableCustomLineHeight'] = get_theme_support( 'experimental-line-height' );
return $settings;
}
add_filter( 'block_editor_settings', 'gutenberg_extend_settings_custom_line_height' );
Expand Down
2 changes: 1 addition & 1 deletion packages/block-editor/src/hooks/line-height.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ export function useIsLineHeightDisabled( { name: blockName } = {} ) {
const isDisabled = useSelect( ( select ) => {
const editorSettings = select( 'core/block-editor' ).getSettings();

return editorSettings.__experimentalDisableCustomLineHeight;
return ! editorSettings.__experimentalEnableCustomLineHeight;
} );

return (
Expand Down
2 changes: 1 addition & 1 deletion packages/editor/src/components/provider/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,7 @@ class EditorProvider extends Component {
'__experimentalBlockPatterns',
'__experimentalBlockPatternCategories',
'__experimentalDisableCustomUnits',
'__experimentalDisableCustomLineHeight',
'__experimentalEnableCustomLineHeight',
'__experimentalEnableCustomSpacing',
'__experimentalEnableLegacyWidgetBlock',
'__experimentalEnableLinkColor',
Expand Down