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

Add Table of Contents block #21040

Closed
wants to merge 39 commits into from
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
39 commits
Select commit Hold shift + click to select a range
e5bea61
Copy changes from pull request #15426 (https://github.com/WordPress/g…
SeanDS Mar 20, 2020
ba40205
Update packages/block-library/src/table-of-contents/utils.js
SeanDS Mar 20, 2020
ba5c43a
Update packages/block-library/src/table-of-contents/utils.js
SeanDS Mar 20, 2020
8507968
Update packages/block-library/src/table-of-contents/utils.js
SeanDS Mar 20, 2020
50df5b5
Update packages/block-library/src/table-of-contents/utils.js
SeanDS Mar 20, 2020
4c650b5
Update packages/block-library/src/table-of-contents/utils.js
SeanDS Mar 20, 2020
371a901
Merge branch 'master' into add/table-of-contents
SeanDS Mar 24, 2020
d2e929b
Merge branch 'master' into add/table-of-contents
SeanDS Mar 25, 2020
c0f2e1a
Fix variable name capitalisation
SeanDS Mar 26, 2020
4fce9a1
Rename function
SeanDS Mar 26, 2020
cb3e495
Reword description
SeanDS Mar 26, 2020
bcfc383
Change argument from array to headingsList
SeanDS Mar 26, 2020
f6aeb36
Change variable name
SeanDS Mar 26, 2020
366d7bd
Add comment
SeanDS Mar 26, 2020
0648776
Get rid of autosync (users should now convert to list if they want to…
SeanDS Mar 26, 2020
29e3c05
Add ability to transform into list; remove unused ListLevel props
SeanDS Mar 26, 2020
5cfade2
Update table-of-contents block test configuration
SeanDS Mar 26, 2020
b89368d
Simplify expression
SeanDS Mar 26, 2020
b82a9e5
Remove unused function
SeanDS Mar 26, 2020
7e4a236
Remove unused style
SeanDS Mar 26, 2020
10d49be
Remove unnecessary style
SeanDS Mar 27, 2020
32a376a
Rename TOCEdit to TableOfContentsEdit
SeanDS Mar 27, 2020
e49f28d
Apply suggestions from code review
SeanDS Mar 27, 2020
becb1a9
Merge branch 'add/table-of-contents' of github.com:SeanDS/gutenberg i…
SeanDS Mar 27, 2020
e7f3dae
Remove non-existent import
SeanDS Mar 27, 2020
8cbe8fc
Make imports explicit
SeanDS Mar 27, 2020
36adc5b
Remove unused function
SeanDS Mar 27, 2020
45ee5f7
Change unsubscribe function to class property
SeanDS Mar 27, 2020
04e0df2
Change JSON.stringify comparison to Lodash's isEqual
SeanDS Mar 27, 2020
afacffc
Turns out refresh() is required
SeanDS Mar 27, 2020
796900a
Remove unnecessary state setting
SeanDS Mar 27, 2020
d31534c
Don't change state on save
SeanDS Mar 27, 2020
6d1d82c
Change behaviour to only add links if there are anchors specified by …
SeanDS Mar 27, 2020
a96a5dc
Newline
SeanDS Mar 27, 2020
00255a0
Replace anchor with explicit key in map since anchor can now sometime…
SeanDS Mar 27, 2020
af0f900
Update test data
SeanDS Mar 27, 2020
9c73d7b
Update packages/block-library/src/table-of-contents/block.json
SeanDS Mar 28, 2020
99bbe0b
Rename ListLevel to ListItem for clarity
SeanDS Mar 28, 2020
d9037e1
Update packages/block-library/src/table-of-contents/ListItem.js
draganescu Apr 2, 2020
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
1 change: 1 addition & 0 deletions packages/block-library/src/editor.scss
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@
@import "./spacer/editor.scss";
@import "./subhead/editor.scss";
@import "./table/editor.scss";
@import "./table-of-contents/editor.scss";
@import "./tag-cloud/editor.scss";
@import "./template-part/editor.scss";
@import "./text-columns/editor.scss";
Expand Down
2 changes: 2 additions & 0 deletions packages/block-library/src/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@ import * as shortcode from './shortcode';
import * as spacer from './spacer';
import * as subhead from './subhead';
import * as table from './table';
import * as tableOfContents from './table-of-contents';
import * as textColumns from './text-columns';
import * as verse from './verse';
import * as video from './video';
Expand Down Expand Up @@ -150,6 +151,7 @@ export const registerCoreBlocks = () => {
spacer,
subhead,
table,
tableOfContents,
tagCloud,
textColumns,
verse,
Expand Down
74 changes: 74 additions & 0 deletions packages/block-library/src/table-of-contents/ListLevel.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,74 @@
/**
* WordPress dependencies
*/
import { RichText } from '@wordpress/editor';

export default function ListLevel( props ) {
SeanDS marked this conversation as resolved.
Show resolved Hide resolved
const { edit, attributes, setAttributes } = props;
let childnodes = null;
SeanDS marked this conversation as resolved.
Show resolved Hide resolved

if ( props.children ) {
childnodes = props.children.map( function( childnode ) {
const link = getLinkElement( childnode, props );

return (
<li key={ childnode.block.anchor }>
{ link }
{ childnode.children ? (
<ListLevel
edit={ edit }
attributes={ attributes }
setAttributes={ setAttributes }
>
{ childnode.children }
</ListLevel>
) : null }
</li>
);
} );

return <ul>{ childnodes }</ul>;
}
}

function getLinkElement( childnode, props ) {
const { edit, attributes, setAttributes } = props;
const { headings, autosync } = attributes;

const updateHeading = ( content ) => {
headings[ childnode.index ].content = content;
setAttributes( { headings } );
};

if ( autosync ) {
return (
<a
href={ childnode.block.anchor }
data-level={ childnode.block.level }
>
{ childnode.block.content }
</a>
);
}

if ( edit ) {
return (
<RichText
tagName="a"
href={ childnode.block.anchor }
data-level={ childnode.block.level }
onChange={ ( content ) => updateHeading( content ) }
value={ childnode.block.content }
/>
);
}

return (
<RichText.Content
tagName="a"
href={ childnode.block.anchor }
data-level={ childnode.block.level }
value={ childnode.block.content }
/>
);
}
4 changes: 4 additions & 0 deletions packages/block-library/src/table-of-contents/block.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
{
"name": "core/table-of-contents",
"category": "common"
}
SeanDS marked this conversation as resolved.
Show resolved Hide resolved
134 changes: 134 additions & 0 deletions packages/block-library/src/table-of-contents/edit.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,134 @@
/**
* Internal dependencies
*/
import * as Utils from './utils';
SeanDS marked this conversation as resolved.
Show resolved Hide resolved
import ListLevel from './ListLevel';

/**
* WordPress dependencies
*/
import { Component } from '@wordpress/element';
import { subscribe } from '@wordpress/data';
import { __ } from '@wordpress/i18n';
import {
IconButton,
Toolbar,
PanelBody,
ToggleControl,
} from '@wordpress/components';
import { BlockControls, InspectorControls } from '@wordpress/editor';

class TOCEdit extends Component {
SeanDS marked this conversation as resolved.
Show resolved Hide resolved
constructor() {
super( ...arguments );

this.state = {
wpDataUnsubscribe: null,
};

this.toggleAttribute = this.toggleAttribute.bind( this );
this.refresh = this.refresh.bind( this );
}

toggleAttribute( propName ) {
SeanDS marked this conversation as resolved.
Show resolved Hide resolved
const value = this.props.attributes[ propName ];
const { setAttributes } = this.props;

setAttributes( { [ propName ]: ! value } );
}

refresh() {
const { setAttributes } = this.props;
const headings = Utils.getPageHeadings();
Copy link
Contributor

Choose a reason for hiding this comment

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

There are a lot of separate calls to getPageHeadings:

  • constructor
  • componentDidMount
  • componentDidUpdate

I think those could probably be reduced down to one or two places so it's a bit clearer how much computation is going on.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I got rid of refresh altogether - it was left over from some removed code. Now getPageHeadings (now called getHeadingsList) is only used in componentDidMount.

setAttributes( { headings } );
}

componentDidMount() {
const { attributes, setAttributes } = this.props;
const headings = attributes.headings || [];
const wpDataUnsubscribe = subscribe( () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Looking at the implementation of getPageHeadings, it returns a new array after mapping getBlocks every time and that's passed to setState, so the block re-renders in the editor every time subscribe triggers, which is any change within the editor.

This block will have to be really careful to guard against endlessly cascading updates as well, since this block listens for changes to other blocks, but also automatically updates other blocks (itself and other headers) so that could cause an endless spiral of updates! Seems pretty risky.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've dealt with your second point by removing any ability to change other blocks altogether. The first point is probably still a problem. Please take a look at the latest version and let me know if you have a suggestion on avoiding re-renders on every trigger.

const pageHeadings = Utils.getPageHeadings();
this.setState( { pageHeadings } );
} );

setAttributes( { headings } );
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm a bit unclear what this does. It seems to set the headings attribute to the same value it already is. Might be possible to delete it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I had to keep that there to make the block work when newly created, but I haven't checked again since changing a bunch of other things. I can experiment removing it to see if it still behaves.

this.setState( { wpDataUnsubscribe } );
SeanDS marked this conversation as resolved.
Show resolved Hide resolved
}

componentWillUnmount() {
this.state.wpDataUnsubscribe();
}

componentDidUpdate( prevProps, prevState ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not completely sure about the need to use componentDidUpdate, it seems like this could go in the subscribe callback. It's good to reduce component updates/renders, and it seems like using componentDidUpdate will result in two successive renders.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is no longer updating state so (I think?) doesn't trigger a re-render. It might still be possible to merge into subscribe though - what do you think?

const { attributes, setAttributes } = this.props;
const pageHeadings = Utils.getPageHeadings();
SeanDS marked this conversation as resolved.
Show resolved Hide resolved
if (
JSON.stringify( pageHeadings ) !==
JSON.stringify( prevState.pageHeadings )
) {
SeanDS marked this conversation as resolved.
Show resolved Hide resolved
this.setState( { pageHeadings } );
SeanDS marked this conversation as resolved.
Show resolved Hide resolved
if ( attributes.autosync ) {
setAttributes( { headings: pageHeadings } ); // this is displayed on the page
SeanDS marked this conversation as resolved.
Show resolved Hide resolved
}
}
}

render() {
const { attributes, setAttributes } = this.props;
const { autosync } = attributes;
const headings = attributes.headings || [];
if ( headings.length === 0 ) {
return (
<p>
{ __(
'Start adding headings to generate Table of Contents'
SeanDS marked this conversation as resolved.
Show resolved Hide resolved
) }
</p>
);
}

Utils.updateHeadingBlockAnchors();

return (
<div className={ this.props.className }>
{ ! autosync && (
<BlockControls>
<Toolbar>
<IconButton
label={ __( 'Update' ) }
aria-pressed={ this.state.isEditing }
onClick={ this.refresh }
icon="update"
/>
</Toolbar>
</BlockControls>
) }
{
<InspectorControls>
<PanelBody title={ __( 'Table of Contents Settings' ) }>
<ToggleControl
label={ __( 'Auto Sync' ) }
SeanDS marked this conversation as resolved.
Show resolved Hide resolved
checked={ autosync }
onChange={ () => {
if ( ! autosync ) {
this.refresh();
}
this.toggleAttribute( 'autosync' );
} }
/>
</PanelBody>
</InspectorControls>
}
<ListLevel
edit={ true }
attributes={ attributes }
SeanDS marked this conversation as resolved.
Show resolved Hide resolved
setAttributes={ setAttributes }
SeanDS marked this conversation as resolved.
Show resolved Hide resolved
>
{ Utils.linearToNestedList( headings ) }
</ListLevel>
</div>
);
}
}

export default TOCEdit;
21 changes: 21 additions & 0 deletions packages/block-library/src/table-of-contents/editor.scss
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
.wp-block-table-of-contents {
.editor-block-list__block & ul {
padding-left: 1.3em;

a {
display: block;

&:focus {
box-shadow: none;
}
}
SeanDS marked this conversation as resolved.
Show resolved Hide resolved

ul {
margin-bottom: 0;
}
}

p {
opacity: 0.5;
SeanDS marked this conversation as resolved.
Show resolved Hide resolved
}
}
41 changes: 41 additions & 0 deletions packages/block-library/src/table-of-contents/index.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
/**
* WordPress dependencies
*/
import { __ } from '@wordpress/i18n';

/**
* Internal dependencies
*/
import edit from './edit';
import metadata from './block.json';
import save from './save';

const { name } = metadata;

export { metadata, name };

export const settings = {
title: __( 'Table of Contents' ),
description: __(
'Add a list of internal links allowing your readers to quickly navigate around.'
Copy link
Member

Choose a reason for hiding this comment

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

I think this description needs to be updated since the block no longer automatically adds anchors to the headings.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True. Maybe "Add a list of headers allowing your readers to quickly navigate through your post." instead, and somewhere else make it obvious to users how to make them links (maybe with popovers or similar)?

Copy link
Member

@ZebulanStanphill ZebulanStanphill Mar 28, 2020

Choose a reason for hiding this comment

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

I'd say "headings" rather than "headers". Also, the contributor guide recommends avoiding describing features as "allowing" something.

How's this?

A list of headings to summarize your post. Add HTML anchors to Heading blocks to automatically link them here.

You might be able to leave off "automatically".

),
icon: 'list-view',
category: 'layout',
attributes: {
headings: {
source: 'query',
selector: 'a',
query: {
content: { source: 'text' },
anchor: { source: 'attribute', attribute: 'href' },
level: { source: 'attribute', attribute: 'data-level' },
},
},
autosync: {
type: 'boolean',
default: true,
},
},
edit,
save,
};
27 changes: 27 additions & 0 deletions packages/block-library/src/table-of-contents/save.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
/**
* Internal dependencies
*/
import * as Utils from './utils';
SeanDS marked this conversation as resolved.
Show resolved Hide resolved
import ListLevel from './ListLevel';

export default function save( props ) {
const { attributes, setAttributes } = props;
const headings = attributes.headings;

if ( headings.length === 0 ) {
return null;
}

Utils.updateHeadingBlockAnchors();
Copy link
Contributor

Choose a reason for hiding this comment

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

It's unusual to see a function with side effects being called in a save function. save functions should ideally be pure.

It looks like this function updates other heading blocks on save, setting anchors on headings that don't have them. I don't think that's a good idea as it's not clear to the user that this has happened. That it also happens on save is less than ideal, most users don't expect their content to be changed at this point.

I think it'd be good to engage the design team here about how to tackle this. It might be that the block alerts the user to headings that don't have anchors. Maybe the block also provides a system for automatically updating those anchors in its editor interface. This is fairly new territory for core blocks so far. I think there's been exploration for parents updating child blocks, but not so much siblings updating other siblings.

For the short-term, my feeling is that it might be worth removing this functionality from the block, and displaying headers without anchors as plain text instead of a link. I think the block could be shipped like that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I really like this suggestion, since it removes any guess work and requires explicit user input for entries in the table of contents to become links. I've changed the code to work this way, and it has simplified it a great deal. Now the block does not change any other components.

return (
<nav className={ props.className }>
<ListLevel
edit={ false }
attributes={ attributes }
setAttributes={ setAttributes }
>
{ Utils.linearToNestedList( headings ) }
</ListLevel>
</nav>
);
}
Loading