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.

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

Display heading levels dynamically in Heading Options when Heading block is selected #14689

Closed
wants to merge 7 commits into from
Closed

Display heading levels dynamically in Heading Options when Heading block is selected #14689

wants to merge 7 commits into from

Conversation

igmoweb
Copy link

@igmoweb igmoweb commented Mar 28, 2019

Description

  1. public_html/wp-content/plugins/gutenberg/packages/block-library/src/heading/edit.js now parses the heading selector choices into numbers and dynamically display them as selector options in sidebar and toolbar.
  2. public_html/wp-content/plugins/gutenberg/packages/block-library/src/heading/heading-toolbar.js (HeadingToolbar component) now accepts another parameter, levelsRange, that allows passing a list of levels instead of min/max values. This would allow passing heading lists like H1, H3, H5... instead of the common H1, H2, H3, H4, H5.
  3. If the levelsRange prop is preset, the minLevel and the maxLevel props will be ignored. I couldn't come up with another solution to keep backward compatibility for other developers that could be using this component in their own JS.

How has this been tested?

  • Create a new post and add a new heading block.
  • Use the following code to change the heading selector options:
wp.hooks.addFilter( 'blocks.registerBlockType', 'heading-blocks-settings', ( settings, blockName ) => {
	if ( blockName !== 'core/heading' ) {
		return settings;
	}

	const { attributes = {} } = settings;
	return {
		...settings,
		attributes: {
			...attributes,
			content: {
				...attributes.content,
				selector: 'h2,h3,h4',
			},
		},
	};
} );

And you should see just a subset of headings to choose:

Captura de pantalla 2019-03-28 a las 18 28 37

Types of changes

New feature (non-breaking change which adds functionality)

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.

@gziolo
Copy link
Member

gziolo commented Mar 29, 2019

@WordPress/gutenberg-core that's a very interesting proposal. On one side, I like how it automates the handling of the heading levels. On the other hand, I don't think that updating the list of selectors is a good idea as it might invalidate Heading block in the previously stored blocks if they used removed hX tag names.

However, this PR gave me an idea that in addition to values of attributes we might want to pass down the schema definition to make it possible to use some metadata in the block. In this particular example, we could do the following change to attributes:

	level: {
		type: 'number',
		default: 2,
+ 		enum: [ 2, 3, 4 ],
	},

And use enum to filter out possible values of controls.

@igmoweb
Copy link
Author

igmoweb commented Mar 29, 2019

@gziolo Thanks for your response.

Whether updating the list of headings is a good idea or not, it's more up to a project specs. In our case, we need to prevent editors to use H1, H5 and H6 in their posts for consistency and SEO matters. I know that this change could invalidate the content of the block but that's something we're having in mind.

On the other hand, changing attributes by using blocks.registerBlockType filter is a reality and can be done easily so in my opinion, it would be up to the developer to know what's being done.

That said, I've updated the PR with your recommendation about the enum property so now the headings are displayed based on that property instead of the selectors inside content.

Copy link
Member

@aduth aduth left a comment

Choose a reason for hiding this comment

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

I like the implementation here in rendering based on the component's own enum value. The filter mechanic is maybe not the nicest experience for extending, but it seems like the best option currently available. It could probably do with some complementing documentation / tutorial for customizing the block options.

However, as I consider it more, it's worth highlighting that enum validation is not currently implemented in the browser (it is for server block registration). I fear that if this were to become implemented and values outside the limited enum range were to become rejected, we'd face the same issue of previous block content being marked as invalidated.

} ) {
const { align, content, level, placeholder } = attributes;
const tagName = 'h' + level;

const BlockControlsLevelsRange = ( levelChoices.length <= 3 ) ?
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Author

Choose a reason for hiding this comment

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

Fixed


const BlockControlsLevelsRange = ( levelChoices.length <= 3 ) ?
levelChoices :
levelChoices.slice( 1, 4 ); //
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure I'm following what the logic is here as far as the second, third, fourth, and fifth level choice options being significant. I sense you'd considered to add an inline comment but forgot to complete it?

Taking a guess with the default options, is it mostly about limiting to a select few common options, and omitting H1 specifically? In that case it seems more accurate to express as something like take( without( levelChoices, [ 1 ] ), 3 ) (Array#slice can work just as well)

https://lodash.com/docs/4.17.11#take
https://lodash.com/docs/4.17.11#without

This also seems like it would help avoid having to test length <= 3 and just have a single consistent expression for computing the level choices.

Copy link
Author

Choose a reason for hiding this comment

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

@aduth What I'm trying to do here is to replicate the previous behavior in the BlockControls options (not the sidebar). Right now, it displays only 3 headings: H2, H3 and H4. So if the levels list contains 3 or less options, it will display all of them, otherwise, slice the array and display 3 elements starting by the second one. I think using Lodash for that would be an overkill for such a simple operation.

packages/block-library/src/heading/edit.js Outdated Show resolved Hide resolved
</BlockControls>
<InspectorControls>
<PanelBody title={ __( 'Heading Settings' ) }>
<p>{ __( 'Level' ) }</p>
<HeadingToolbar minLevel={ 1 } maxLevel={ 7 } selectedLevel={ level } onChange={ ( newLevel ) => setAttributes( { level: newLevel } ) } />
Copy link
Member

Choose a reason for hiding this comment

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

Did the default max level change from 7 to 6? And if so, why?

Copy link
Author

Choose a reason for hiding this comment

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

);

return {
levelChoices,
Copy link
Member

Choose a reason for hiding this comment

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

This implementation will result in poor performance, at least when using the fallback empty array value, due to a combination of factors that (a) withSelect is called for any change in application state, (b) you are producing a new array reference value ([] !== []), and (c) a render is forced for any new prop value reference.

As long as a block types attributes.level.enum remains a consistent reference (I think it's safe to assume), then a simple adjustment here may be to move the array default to the top of the file as a shared constant:

const DEFAULT_LEVEL_CHOICES = [ 1, 2, 3, 4, 5, 6, 7 ];

Copy link
Author

Choose a reason for hiding this comment

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

Thanks! Great point. It's fixed now.

Just a side comment: The previous maxLevel was set to 7 but that's confusing, HeadingToolbar component makes use of Lodash range https://lodash.com/docs/4.17.11#range and when it generates the range, the second argument passed to the function (7) is not included in the range. That's why we only need 1-6 as default values.

@igmoweb
Copy link
Author

igmoweb commented Apr 3, 2019

Thanks @aduth, I've pushed some changes.

However, as I consider it more, it's worth highlighting that enum validation is not currently implemented in the browser (it is for server block registration). I fear that if this were to become implemented and values outside the limited enum range were to become rejected, we'd face the same issue of previous block content being marked as invalidated.

I don't know if I'm following you 100% but I'll use an example:

  • Create a heading and use H4. Save the post.
  • Via filters, remove the number 4 from the enum attributes.
  • Refresh the post, the H4 option dissapears but the heading is still wrapped in a H4. If you don't change it again, it will remain like that.

Depending on how enum is validated in the future, the block could suffer the invalidation problem.

@aduth
Copy link
Member

aduth commented Apr 3, 2019

Depending on how enum is validated in the future, the block could suffer the invalidation problem.

Yeah, that's the crux of it. We don't have the validation today for blocks registered in the browser. The default behavior for server-registered blocks using rest_validate_value_from_schema is to outright omit the attribute value if it was not valid per the schema (source). This does include enum validation (source). I expect the equivalent behavior translated to the browser would result in a block invalidation if that 4 value would not be included in the enum set after your filtering.

It's also increasingly seeming like something which should be aligned with the server validation sooner than later, given the potential for unintended block invalidations (though enum usage is probably quite limited and not officially documented). Doing so could also help validate the behavior here. I could see if I can set some time aside to implement it so we can test against it in the next days, unless you have some interest or thoughts on how that should be handled.

@igmoweb
Copy link
Author

igmoweb commented Apr 3, 2019

@aduth This can wait a bit more if you need so and test with that enum code. I don't know if it would be interesting to fallback to the first enum option if a wrong one is selected. It's not too elegant but it would help to not see the block invalidated, which would be way worse.

@aduth
Copy link
Member

aduth commented Apr 3, 2019

@aduth This can wait a bit more if you need so and test with that enum code. I don't know if it would be interesting to fallback to the first enum option if a wrong one is selected. It's not too elegant but it would help to not see the block invalidated, which would be way worse.

I think a fallback to a default value would be expected, if one is specified (this occurs server-side). I sense it's still an issue however in how the block validation works: Following the same example, if the default was 2, the block would be marked as invalid because the editor would yield a different result (<h2>Hello World</h2>) from the original saved post (<h4>Hello World</h4>) when initializing.

https://wordpress.org/gutenberg/handbook/designers-developers/developers/block-api/block-edit-save/#validation

@igmoweb
Copy link
Author

igmoweb commented Apr 5, 2019

@aduth That's correct. I don't really know how you handle these cases in Gutenberg, do you think that this case would be more of a developer responsibility or should we use another approach?

@gziolo
Copy link
Member

gziolo commented Apr 12, 2019

We landed #14810 where @aduth implemented clien-side validation for enum.

@gziolo gziolo mentioned this pull request Apr 12, 2019
5 tasks
@aduth
Copy link
Member

aduth commented Apr 29, 2019

To give some update so this doesn't stagnate: I don't think we can realistically leverage extending a block's enum for this, due to aforementioned risk of unintended block invalidation and the changes brought with #14810. I'm not really sure yet what an alternative extensibility pattern here would look like, but it's a similar need which has been raised in a few related issues for sidebar extensibility. Neither of the current extensibility patterns of Slot/Fill nor filters seem like a good/advisable fit. The Roadmap entry of "Common Block Functionality" seems maybe promising here as far as generally needing some filtering options for themes to constrain visible options (e.g. alignment, but seems also reasonable for this sort of implementation).

@gziolo
Copy link
Member

gziolo commented May 6, 2019

The Roadmap entry of "Common Block Functionality" seems maybe promising here as far as generally needing some filtering options for themes to constrain visible options (e.g. alignment, but seems also reasonable for this sort of implementation).

I opened #15450 and linked this PR to ensure it is included in the discussion about Common Block Functionality.

@gziolo gziolo added the First-time Contributor Pull request opened by a first-time contributor to Gutenberg repository label Jun 3, 2019
@gziolo
Copy link
Member

gziolo commented Nov 30, 2020

@jorgefilipecosta and @nosolosw – any thoughts on how this sort of block specific attributes could be controlled with Global Styles or block editor features mechanics?

@jorgefilipecosta
Copy link
Member

@jorgefilipecosta and @nosolosw – any thoughts on how this sort of block specific attributes could be controlled with Global Styles or block editor features mechanics?

If we see heading levels as block specific attribute, I don't think it should be a setting present in global styles/ block editor features. But if we see heading levels as a global setting that may be used by multiple blocks (heading block, site title block, post title, a custom heading implementation, etc), I think it may make sense to add the setting to Global Styles, similarly to how we have a drop cap setting.

@youknowriad youknowriad closed this Mar 1, 2021
@youknowriad youknowriad deleted the branch WordPress:master March 1, 2021 15:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Heading Affects the Headings Block First-time Contributor Pull request opened by a first-time contributor to Gutenberg repository [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants