-
Notifications
You must be signed in to change notification settings - Fork 4.6k
Add Heading level variations #73823
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
Add Heading level variations #73823
Changes from all commits
8135742
25d5818
1cd6756
fb93572
e6a6074
f079360
a6ab7af
82b3673
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,7 +3,11 @@ | |
| */ | ||
| import { heading as icon } from '@wordpress/icons'; | ||
| import { __, sprintf } from '@wordpress/i18n'; | ||
| import { privateApis as blocksPrivateApis } from '@wordpress/blocks'; | ||
| import { | ||
| privateApis as blocksPrivateApis, | ||
| getBlockType, | ||
| unregisterBlockVariation, | ||
| } from '@wordpress/blocks'; | ||
|
|
||
| /** | ||
| * Internal dependencies | ||
|
|
@@ -86,4 +90,21 @@ if ( window.__experimentalContentOnlyInspectorFields ) { | |
| }; | ||
| } | ||
|
|
||
| export const init = () => initBlock( { name, metadata, settings } ); | ||
| export const init = () => { | ||
| const block = initBlock( { name, metadata, settings } ); | ||
|
|
||
| // Unregister heading level variations based on `levelOptions` attribute. | ||
| // This is for backwards compatibility, as extenders can now unregister the | ||
| // variation directly: `wp.blocks.unregisterBlockVariation( 'core/heading', 'h1' )`. | ||
| const levelOptions = | ||
| getBlockType( name )?.attributes?.levelOptions?.default; | ||
| if ( levelOptions ) { | ||
| [ 1, 2, 3, 4, 5, 6 ].forEach( ( level ) => { | ||
| if ( ! levelOptions.includes( level ) ) { | ||
| unregisterBlockVariation( name, `h${ level }` ); | ||
| } | ||
| } ); | ||
| } | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This works and I think it's totally fine for now but: changing a "default" attribute value to define what levels are available seems like a bad thing to start with. I wonder if we should deprecate that attribute. Unregistering a variation seems like a way better approach.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That would break sites that rely on the default options attribute to remove the heading levels. I think @fabiankaegy is also suggesting keeping the toolbar dropdown item - #73823 (comment).
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is why I said "deprecate" and not "remove" :)
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That said, reading @fabiankaegy's comment I do agree that at a "pattern" level it's a good idea to allow disabling level options and things like that. |
||
|
|
||
| return block; | ||
| }; | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a new flag right? I'm a little bit hesitant about it.
I mean "scope" have been used in variations to define where a variation should appear. And by adding this flag we now have two properties to define that. I may have a small preference for another "search" scope.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not being set by the consumer, it's just an internal flag to explicitly treat "block" context to show up in search. I implemented the search scope initially but it seemed unnecessary and confusing, since you'd think you need to pass the extra search scope to ensure something shows up on searches, but "inserter" already handles that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah ok, maybe we can just pass down the scope or something no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Regardless, I'm way less concerned now :)