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

Autogenerate heading anchors #30825

Merged
merged 64 commits into from
Oct 14, 2021
Merged
Show file tree
Hide file tree
Changes from 56 commits
Commits
Show all changes
64 commits
Select commit Hold shift + click to select a range
5cb5c9a
auto-generate anchors for heading blocks
aristath Apr 13, 2021
ca2f664
Don't use "we" & "us" in comments
aristath Apr 14, 2021
556f82b
account for non-latin anchors
aristath Apr 14, 2021
4a6acd4
remove lodash dependency
aristath Apr 14, 2021
042a1aa
Revert "remove lodash dependency"
aristath Apr 14, 2021
8c4be85
add dom-ready dependency
aristath Apr 14, 2021
766ef32
refactor everything
aristath Apr 15, 2021
3296651
use useSelect
aristath Apr 16, 2021
3e6928c
This was not removed on purpose
aristath Apr 16, 2021
1e5d3d7
Move dummyElement inside getTextWithoutMarkup
aristath Apr 16, 2021
2d20135
Update e2e tests
aristath Apr 19, 2021
69efbab
Don't use a plain "wp-" as anchor
aristath Apr 19, 2021
e5af9c8
typo
aristath Apr 19, 2021
15e04db
Improve empty string handling
aristath Apr 19, 2021
635eca9
Improve inline comments
aristath Apr 19, 2021
4c03e1b
Bugfix anchor generation for empty headings
aristath Apr 19, 2021
abeaaaf
Update packages/block-library/src/heading/edit.js
aristath Apr 20, 2021
9f728c2
Use blockEditorStore
aristath Apr 20, 2021
4579a72
cs
aristath Apr 20, 2021
93b0d4d
refactor hook
aristath Apr 20, 2021
a9f6381
Update packages/block-library/src/heading/edit.js
aristath Apr 22, 2021
762af2c
no need for lodash here, these are pretty simple
aristath Apr 22, 2021
c3ccf1c
refactor - no prefix
aristath Apr 22, 2021
47014dd
update e2e
aristath Apr 23, 2021
cc34994
bugfix
aristath Apr 23, 2021
4b9e037
simplify
aristath Apr 23, 2021
a1263d9
fix for block transforms
aristath Apr 23, 2021
370409e
Improve generated slugs
aristath Apr 26, 2021
951480c
bring back the prefix
aristath Apr 26, 2021
9953f70
Revert "update e2e"
aristath Apr 23, 2021
694180e
remove wp- prefix if not autogenerated anchor
aristath Apr 26, 2021
34d1721
fix for empty/null anchors
aristath Apr 26, 2021
50141d7
use useEffect properly
aristath Apr 26, 2021
7e0f1d9
Merge branch 'trunk' into add/autogenerate-heading-anchors
aristath May 5, 2021
f1ad7c2
Refactor to not use a prefix.
aristath May 5, 2021
c61d9a7
Maybe not use setAttributes? :thinking:
aristath May 5, 2021
7d8c3ea
no need for this condition
aristath May 5, 2021
00006fe
This was too ugly to stay for more than 10 minutes
aristath May 5, 2021
7b8b783
remove prefix from tests
aristath May 5, 2021
516c400
alternative to useEffect
aristath May 5, 2021
842449d
Merge branch 'trunk' into add/autogenerate-heading-anchors
aristath May 5, 2021
3ec7e1e
Revert "alternative to useEffect"
aristath May 5, 2021
171f228
only watch the content
aristath May 5, 2021
577feb6
mark anchor changes as not persistent
aristath May 5, 2021
5706771
yet another refactor
aristath May 5, 2021
60114f1
fix for legacy headers & conversions
aristath May 5, 2021
0dbefeb
Typo was causing a crash
aristath May 5, 2021
2a021a6
use useEffect for side effects
ntsekouras May 6, 2021
716b5a9
Merge branch 'trunk' into add/autogenerate-heading-anchors
aristath May 10, 2021
4f5c06e
Merge branch 'trunk' into add/autogenerate-heading-anchors
aristath May 20, 2021
88cf66c
Merge branch 'trunk' into add/autogenerate-heading-anchors
aristath May 26, 2021
8c0e353
Merge branch 'trunk' into add/autogenerate-heading-anchors
aristath Jun 2, 2021
234abfc
Merge branch 'trunk' into add/autogenerate-heading-anchors
aristath Jun 14, 2021
6535526
Merge branch 'trunk' into add/autogenerate-heading-anchors
aristath Jun 18, 2021
8f39085
Merge branch 'trunk' into add/autogenerate-heading-anchors
aristath Jun 30, 2021
43cc150
Merge branch 'trunk' into add/autogenerate-heading-anchors
aristath Jul 8, 2021
db12f78
Merge branch 'trunk' into add/autogenerate-heading-anchors
aristath Jul 19, 2021
5bb9302
Simplify & improve performance
aristath Jul 20, 2021
bbabee8
Merge branch 'trunk' into add/autogenerate-heading-anchors
aristath Jul 20, 2021
98d29dc
Use an object instead of Set
aristath Jul 21, 2021
b5cb1f9
Merge branch 'trunk' into add/autogenerate-heading-anchors
aristath Sep 6, 2021
2b1f215
Merge branch 'trunk' into add/autogenerate-heading-anchors
aristath Sep 14, 2021
79f49ff
Merge branch 'trunk' into add/autogenerate-heading-anchors
aristath Sep 28, 2021
b5b34a8
Merge branch 'trunk' into add/autogenerate-heading-anchors
aristath Oct 7, 2021
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
127 changes: 127 additions & 0 deletions packages/block-library/src/heading/autogenerate-anchors.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,127 @@
/**
* External dependencies
*/
import { deburr, trim } from 'lodash';

/**
* WordPress dependencies
*/
import { useSelect } from '@wordpress/data';
import { store as blockEditorStore } from '@wordpress/block-editor';

/**
* Runs a callback over all blocks, including nested blocks.
*
* @param {Object[]} blocks The blocks.
* @param {Function} callback The callback.
*
* @return {void}
*/
const recurseOverBlocks = ( blocks, callback ) => {
for ( const block of blocks ) {
// eslint-disable-next-line callback-return
callback( block );
aristath marked this conversation as resolved.
Show resolved Hide resolved
if ( block.innerBlocks ) {
recurseOverBlocks( block.innerBlocks, callback );
}
}
};

/**
* Returns the text without markup.
*
* @param {string} text The text.
*
* @return {string} The text without markup.
*/
const getTextWithoutMarkup = ( text ) => {
const dummyElement = document.createElement( 'div' );
dummyElement.innerHTML = text;
return dummyElement.innerText;
};

/**
* Get all heading anchors.
*
* @param {Object} blockList An object containing all blocks.
* @param {string} excludeId A block ID we want to exclude.
*
* @return {string[]} Return an array of anchors.
*/
const getAllHeadingAnchors = ( blockList, excludeId ) => {
const anchors = [];

recurseOverBlocks( blockList, ( block ) => {
if (
block.name === 'core/heading' &&
( ! excludeId || block.clientId !== excludeId ) &&
block.attributes.anchor
) {
anchors.push( block.attributes.anchor );
}
} );

return anchors;
};

/**
* Get the slug from the content.
*
* @param {string} content The block content.
*
* @return {string} Returns the slug.
*/
const getSlug = ( content ) => {
// Get the slug.
return trim(
deburr( getTextWithoutMarkup( content ) )
.replace( /[^\p{L}\p{N}]+/gu, '-' )
.toLowerCase(),
'-'
);
};

/**
* Generate the anchor for a heading.
*
* @param {string} content The block content.
* @param {string[]} allHeadingAnchors An array containing all headings anchors.
*
* @return {string|null} Return the heading anchor.
*/
export const generateAnchor = ( content, allHeadingAnchors ) => {
const slug = getSlug( content );
// If slug is empty, then return null.
// Returning null instead of an empty string allows us to check again when the content changes.
if ( '' === slug ) {
return null;
}

let anchor = slug;
let i = 0;

// If the anchor already exists in another heading, append -i.
while ( allHeadingAnchors.includes( anchor ) ) {
i += 1;
anchor = slug + '-' + i;
}

return anchor;
};
Copy link
Contributor

@adamziel adamziel Jul 13, 2021

Choose a reason for hiding this comment

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

Let's say I save a page with three headings that map to the same anchor. Then here we get plants, plants-1, plants-2. Let's say I remove the initial paragraph from that page, and add a new one that is also titled plants. I think this code would assign it the freshly freed anchor plants, which means that all the links to that anchor would now lead to a wrong heading.

I would love to make these headings persistent in some way. A naive approach would involve always +1-ing the largest related number, so even if plants is free and we only have plants-1 and plants-2, we would still assign plants-3.

The problem with that approach is that it is possible to remove just plants-2 or even both plants-1 and plants-2 so now there is no information we could use to avoid assigning new headings with the same anchors. For that, we would need to store the list of the "largest" anchors for each slug somewhere (["plants-4", "doors-4", "potatoes"]). Maybe in page meta...?

Choose a reason for hiding this comment

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

I think think that this is required or even desired. Because normally you want to avoid duplicated headings. It makes no sense in most cases and might be bad for SEO anyways.

Now imagine I am currently scribbling my post together. I duplicate blocks to try things out, and I remove them afterwards. I end up having id anchor of anchor-2 etc everywhere. But in the end I just want to have a "just working" TOC and a clean link for my users. Thatswhy I would prefer to always fall back to normal headings.

Another note:
What if I have heading and heading-1 and I delete heading. Will heading-1 become heading afterwards. My 2cents are, that it should. Correct me if I am wrong with that idea.

Copy link
Contributor

@adamziel adamziel Jul 13, 2021

Choose a reason for hiding this comment

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

@NicoHood you are right, it does make sense for the writing and drafting phase. What I am worried about is the post-publish phase once the URLs are out there in the wild. Permalinks, cool urls don't change, that kind of stuff.

Choose a reason for hiding this comment

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

@adamziel That is correct, but it is not an url, it is an anchor. Meaning the website will still function, but not scroll.

Lets say you have a heading called: "Red cars" which you rename to "Blue bike". Then the anchor (aka "url") will be "broken" too.

If someone wants a fixed anchor, he should enter his own id from the block settings. I am not sure if that is still possible with this PR, but that's what I'd expect the editor to do.

BTW: That is also how all other CMS work, Github and the Arch Linux Wiki as well. Just be careful in renaming the headings if you are refering to their autogenerated anchor. As easy as it is. I guess there is no "better" or perfect solution.

Copy link
Member Author

Choose a reason for hiding this comment

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

There is no perfect solution here... There are pros and cons in all cases.
I don't think the -1 etc suffixes will be used that often... If they do get used often it's indicative of a messy page structure, since having multiple headings with the same text is sub-optimal for screen-readers. The suffixes are there to avoid issues, but they are an edge-case and not the norm.
Manually-edited anchors are persistent, so if after publishing a page an author edits a page and changes the structure of headings, they can manually edit the anchors and make them whatever they want.


/**
* Retrieves and returns all heading anchors.
*
* @param {string} clientId The block's client-ID.
*
* @return {string[]} The array of heading anchors.
*/
export const useAllHeadingAnchors = ( clientId ) => {
return useSelect(
( select ) => {
const allBlocks = select( blockEditorStore ).getBlocks();
return getAllHeadingAnchors( allBlocks, clientId );
},
[ clientId ]
);
};
29 changes: 27 additions & 2 deletions packages/block-library/src/heading/edit.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import classnames from 'classnames';
* WordPress dependencies
*/
import { __ } from '@wordpress/i18n';
import { useEffect } from '@wordpress/element';
import { createBlock } from '@wordpress/blocks';
import {
AlignmentControl,
Expand All @@ -19,6 +20,7 @@ import {
* Internal dependencies
*/
import HeadingLevelDropdown from './heading-level-dropdown';
import { useAllHeadingAnchors, generateAnchor } from './autogenerate-anchors';

function HeadingEdit( {
attributes,
Expand All @@ -28,14 +30,37 @@ function HeadingEdit( {
style,
clientId,
} ) {
const { textAlign, content, level, placeholder } = attributes;
const { textAlign, content, level, placeholder, anchor } = attributes;
const tagName = 'h' + level;
const blockProps = useBlockProps( {
className: classnames( {
[ `has-text-align-${ textAlign }` ]: textAlign,
} ),
style,
} );
const allHeadingAnchors = useAllHeadingAnchors( clientId );
aristath marked this conversation as resolved.
Show resolved Hide resolved

// Initially set anchor for headings that have content but no anchor set.
// This is used when transforming a block to heading, or for legacy anchors.
useEffect( () => {
if ( ! anchor && content ) {
setAttributes( {
anchor: generateAnchor( content, allHeadingAnchors ),
} );
}
}, [ allHeadingAnchors ] );
aristath marked this conversation as resolved.
Show resolved Hide resolved

const onContentChange = ( value ) => {
const newAttrs = { content: value };
if (
! anchor ||
! value ||
generateAnchor( content, allHeadingAnchors ) === anchor
) {
newAttrs.anchor = generateAnchor( value, allHeadingAnchors );
}
setAttributes( newAttrs );
};

return (
<>
Expand All @@ -57,7 +82,7 @@ function HeadingEdit( {
identifier="content"
tagName={ tagName }
value={ content }
onChange={ ( value ) => setAttributes( { content: value } ) }
onChange={ onContentChange }
onMerge={ mergeBlocks }
onSplit={ ( value, isOriginal ) => {
let block;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,25 +2,25 @@

exports[`Heading can be created by prefixing existing content with number signs and a space 1`] = `
"<!-- wp:heading {\\"level\\":4} -->
<h4>4</h4>
<h4 id=\\"4\\">4</h4>
<!-- /wp:heading -->"
`;

exports[`Heading can be created by prefixing number sign and a space 1`] = `
"<!-- wp:heading {\\"level\\":3} -->
<h3>3</h3>
<h3 id=\\"3\\">3</h3>
<!-- /wp:heading -->"
`;

exports[`Heading should correctly apply custom colors 1`] = `
"<!-- wp:heading {\\"level\\":3,\\"style\\":{\\"color\\":{\\"text\\":\\"#7700ff\\"}}} -->
<h3 class=\\"has-text-color\\" style=\\"color:#7700ff\\">Heading</h3>
<h3 class=\\"has-text-color\\" id=\\"heading\\" style=\\"color:#7700ff\\">Heading</h3>
<!-- /wp:heading -->"
`;

exports[`Heading should correctly apply named colors 1`] = `
"<!-- wp:heading {\\"textColor\\":\\"luminous-vivid-orange\\"} -->
<h2 class=\\"has-luminous-vivid-orange-color has-text-color\\">Heading</h2>
<h2 class=\\"has-luminous-vivid-orange-color has-text-color\\" id=\\"heading\\">Heading</h2>
<!-- /wp:heading -->"
`;

Expand All @@ -30,13 +30,13 @@ exports[`Heading should create a paragraph block above when pressing enter at th
<!-- /wp:paragraph -->

<!-- wp:heading -->
<h2>a</h2>
<h2 id=\\"a\\">a</h2>
<!-- /wp:heading -->"
`;

exports[`Heading should create a paragraph block below when pressing enter at the end 1`] = `
"<!-- wp:heading -->
<h2>a</h2>
<h2 id=\\"a\\">a</h2>
<!-- /wp:heading -->

<!-- wp:paragraph -->
Expand All @@ -46,12 +46,12 @@ exports[`Heading should create a paragraph block below when pressing enter at th

exports[`Heading should not work with the list input rule 1`] = `
"<!-- wp:heading -->
<h2>1. H</h2>
<h2 id=\\"1-h\\">1. H</h2>
<!-- /wp:heading -->"
`;

exports[`Heading should work with the format input rules 1`] = `
"<!-- wp:heading -->
<h2><code>code</code></h2>
<h2 id=\\"code\\"><code>code</code></h2>
<!-- /wp:heading -->"
`;
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ exports[`Quote can be converted to paragraphs and renders only one paragraph for

exports[`Quote can be created by converting a heading 1`] = `
"<!-- wp:quote -->
<blockquote class=\\"wp-block-quote\\"><p>test</p></blockquote>
<blockquote class=\\"wp-block-quote\\" id=\\"test\\"><p>test</p></blockquote>
<!-- /wp:quote -->"
`;

Expand Down Expand Up @@ -144,7 +144,7 @@ exports[`Quote can be split in the middle and merged back 4`] = `

exports[`Quote is transformed to a heading and a quote if the quote contains a citation 1`] = `
"<!-- wp:heading -->
<h2>one</h2>
<h2 id=\\"one\\">one</h2>
<!-- /wp:heading -->

<!-- wp:quote -->
Expand All @@ -154,7 +154,7 @@ exports[`Quote is transformed to a heading and a quote if the quote contains a c

exports[`Quote is transformed to a heading and a quote if the quote contains multiple paragraphs 1`] = `
"<!-- wp:heading -->
<h2>one</h2>
<h2 id=\\"one\\">one</h2>
<!-- /wp:heading -->

<!-- wp:quote -->
Expand All @@ -164,7 +164,7 @@ exports[`Quote is transformed to a heading and a quote if the quote contains mul

exports[`Quote is transformed to a heading if the quote just contains one paragraph 1`] = `
"<!-- wp:heading -->
<h2>one</h2>
<h2 id=\\"one\\">one</h2>
<!-- /wp:heading -->"
`;

Expand All @@ -176,7 +176,7 @@ exports[`Quote is transformed to an empty heading if the quote is empty 1`] = `

exports[`Quote the resuling quote after transforming to a heading can be transformed again 1`] = `
"<!-- wp:heading -->
<h2>one</h2>
<h2 id=\\"one\\">one</h2>
<!-- /wp:heading -->

<!-- wp:quote -->
Expand All @@ -186,11 +186,11 @@ exports[`Quote the resuling quote after transforming to a heading can be transfo

exports[`Quote the resuling quote after transforming to a heading can be transformed again 2`] = `
"<!-- wp:heading -->
<h2>one</h2>
<h2 id=\\"one\\">one</h2>
<!-- /wp:heading -->

<!-- wp:heading -->
<h2>two</h2>
<h2 id=\\"two\\">two</h2>
<!-- /wp:heading -->

<!-- wp:quote -->
Expand All @@ -200,14 +200,14 @@ exports[`Quote the resuling quote after transforming to a heading can be transfo

exports[`Quote the resuling quote after transforming to a heading can be transformed again 3`] = `
"<!-- wp:heading -->
<h2>one</h2>
<h2 id=\\"one\\">one</h2>
<!-- /wp:heading -->

<!-- wp:heading -->
<h2>two</h2>
<h2 id=\\"two\\">two</h2>
<!-- /wp:heading -->

<!-- wp:heading -->
<h2>cite</h2>
<h2 id=\\"cite\\">cite</h2>
<!-- /wp:heading -->"
`;
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
exports[`Block Grouping Group creation creates a group from multiple blocks of different types via block transforms 1`] = `
"<!-- wp:group -->
<div class=\\"wp-block-group\\"><!-- wp:heading -->
<h2>Group Heading</h2>
<h2 id=\\"group-heading\\">Group Heading</h2>
<!-- /wp:heading -->

<!-- wp:image -->
Expand Down Expand Up @@ -51,7 +51,7 @@ exports[`Block Grouping Group creation creates a group from multiple blocks of t
exports[`Block Grouping Group creation groups and ungroups multiple blocks of different types via options toolbar 1`] = `
"<!-- wp:group -->
<div class=\\"wp-block-group\\"><!-- wp:heading -->
<h2>Group Heading</h2>
<h2 id=\\"group-heading\\">Group Heading</h2>
<!-- /wp:heading -->

<!-- wp:image -->
Expand All @@ -66,7 +66,7 @@ exports[`Block Grouping Group creation groups and ungroups multiple blocks of di

exports[`Block Grouping Group creation groups and ungroups multiple blocks of different types via options toolbar 2`] = `
"<!-- wp:heading -->
<h2>Group Heading</h2>
<h2 id=\\"group-heading\\">Group Heading</h2>
<!-- /wp:heading -->

<!-- wp:image -->
Expand All @@ -81,7 +81,7 @@ exports[`Block Grouping Group creation groups and ungroups multiple blocks of di
exports[`Block Grouping Preserving selected blocks attributes preserves width alignment settings of selected blocks 1`] = `
"<!-- wp:group {\\"align\\":\\"full\\"} -->
<div class=\\"wp-block-group alignfull\\"><!-- wp:heading -->
<h2>Group Heading</h2>
<h2 id=\\"group-heading\\">Group Heading</h2>
<!-- /wp:heading -->

<!-- wp:image {\\"align\\":\\"full\\"} -->
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ exports[`Inserting blocks inserts a block in proper place after having clicked \
<!-- /wp:paragraph -->

<!-- wp:heading -->
<h2>Heading</h2>
<h2 id=\\"heading\\">Heading</h2>
<!-- /wp:heading -->

<!-- wp:paragraph -->
Expand All @@ -93,7 +93,7 @@ exports[`Inserting blocks inserts a block in proper place after having clicked \
<!-- /wp:cover -->

<!-- wp:heading -->
<h2>Heading</h2>
<h2 id=\\"heading\\">Heading</h2>
<!-- /wp:heading -->

<!-- wp:paragraph -->
Expand Down
2 changes: 1 addition & 1 deletion packages/e2e-tests/specs/widgets/editing-widgets.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -379,7 +379,7 @@ describe( 'Widgets screen', () => {
<p>First Paragraph</p>
</div></div>
<div class=\\"widget widget_block\\"><div class=\\"widget-content\\">
<h2>My Heading</h2>
<h2 id=\\"my-heading\\">My Heading</h2>
</div></div>
<div class=\\"widget widget_block widget_text\\"><div class=\\"widget-content\\">
<p>Second Paragraph</p>
Expand Down