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

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

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
6791ea6
Dynamically display heading options when Heading Block is selected.
igmoweb Mar 28, 2019
a75eef2
Allow to pass a range to HeadingToolbar component
igmoweb Mar 28, 2019
590592c
Added comment to Heading Edit component
igmoweb Mar 28, 2019
d0ab84a
Simplify Heading Block code with level enum property
igmoweb Mar 29, 2019
e5d5256
Set camel-cased variable
igmoweb Apr 3, 2019
cf484c4
Performance improvements in Heading block
igmoweb Apr 3, 2019
9e71339
Set camel-cased variable
igmoweb Apr 3, 2019
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
31 changes: 27 additions & 4 deletions packages/block-library/src/heading/edit.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,8 @@
/**
* External dependencies
*/
import { get } from 'lodash';

/**
* Internal dependencies
*/
Expand All @@ -10,33 +15,37 @@ import { __ } from '@wordpress/i18n';
import { Fragment } from '@wordpress/element';
import { PanelBody } from '@wordpress/components';
import { createBlock } from '@wordpress/blocks';
import { withSelect } from '@wordpress/data';
import {
RichText,
BlockControls,
InspectorControls,
AlignmentToolbar,
} from '@wordpress/block-editor';

export default function HeadingEdit( {
function HeadingEdit( {
attributes,
setAttributes,
mergeBlocks,
insertBlocksAfter,
onReplace,
className,
levelChoices,
} ) {
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.

This should be camel-cased with an initial lower-case letter.

https://make.wordpress.org/core/handbook/best-practices/coding-standards/javascript/#naming-conventions

Copy link
Author

Choose a reason for hiding this comment

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

Fixed

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.

return (
<Fragment>
<BlockControls>
<HeadingToolbar minLevel={ 2 } maxLevel={ 5 } selectedLevel={ level } onChange={ ( newLevel ) => setAttributes( { level: newLevel } ) } />
<HeadingToolbar levelsRange={ BlockControlsLevelsRange } selectedLevel={ level } onChange={ ( newLevel ) => setAttributes( { level: newLevel } ) } />
</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.

See my comment here: https://github.com/WordPress/gutenberg/pull/14689/files#r271809189

<HeadingToolbar levelsRange={ levelChoices } selectedLevel={ level } onChange={ ( newLevel ) => setAttributes( { level: newLevel } ) } />
<p>{ __( 'Text Alignment' ) }</p>
<AlignmentToolbar
value={ align }
Expand Down Expand Up @@ -72,3 +81,17 @@ export default function HeadingEdit( {
</Fragment>
);
}

export default withSelect( ( select ) => {
// Parse the h1,h2,h3... choices to level numbers and pass it as a prop.
const levelChoices = get(
select( 'core/blocks' ).getBlockType( 'core/heading' ),
[ 'attributes', 'level', 'enum' ],
[]
igmoweb marked this conversation as resolved.
Show resolved Hide resolved
);

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.

};
} )( HeadingEdit );

5 changes: 3 additions & 2 deletions packages/block-library/src/heading/heading-toolbar.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,9 +23,10 @@ class HeadingToolbar extends Component {
}

render() {
const { minLevel, maxLevel, selectedLevel, onChange } = this.props;
const { minLevel, maxLevel, selectedLevel, onChange, levelsRange = [] } = this.props;
const controls = ( levelsRange.length > 0 ) ? levelsRange : range( minLevel, maxLevel );
return (
<Toolbar controls={ range( minLevel, maxLevel ).map( ( index ) => this.createLevelControl( index, selectedLevel, onChange ) ) } />
<Toolbar controls={ controls.map( ( index ) => this.createLevelControl( index, selectedLevel, onChange ) ) } />
);
}
}
Expand Down
1 change: 1 addition & 0 deletions packages/block-library/src/heading/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ const schema = {
level: {
type: 'number',
default: 2,
enum: [ 1, 2, 3, 4, 5, 6 ],
},
align: {
type: 'string',
Expand Down