Autogenerate heading anchors #30825
Autogenerate heading anchors #30825
Conversation
aristath
added
[Block] Heading
|
Size Change: +165 B (0%) Total Size: 1.07 MB
ℹ️ View Unchanged
|
|
👋 Thanks for this PR, I'd love to see headings adopt an anchor by default, will make it more consistent to be able to link deeper inside a post/page!
I wonder if that automatic editor behavior can be a surprise to the user and others. Opening a post can mark the post as dirty (if any heading block auto-generates its anchor) and that might confuse the user, or just end up having involuntary post editions. Perhaps some interaction with the user can be useful? Like, either to notify the user that there has been this automatic change, or even a mild prompt to the user offering to perform this action? Not sure if we have similar interactions actually, automatic content manipulation that either happens silently or includes the user 🤔. @jasmussen perhaps you have insight? |
|
Thanks for the ping. I'm not sure we need to surface it; I'd personally be okay it being dirty when visiting it after an upgrade, mostly because it will only ever be used for existing content, not new content. But if we do need to surface it, I'd think a simple snackbar informing of the change might strike the right balance? |
|
I pushed a couple of bugfixes and also updated the e2e tests. |
There was a problem hiding this comment.
Thanks for working on this Ari! 👍
I've left some small comments and a suggestion(and at the same time question for Riad :) ) about the creation of a new hook.
The only thing besides technical implementation that seems a bit confusing to me is where you have this check startsWith( anchor, 'wp-' ). With this check a user can focus on the input of anchor and can not edit anything after this. For example if we had an auto-generated anchor wp-something, if the user types anything after wp- it seems like it doesn't do anything, because it's regenerating from the content..
Correct. Users can enter a custom anchor if they want, but the |
aristath
force-pushed
the
add/autogenerate-heading-anchors
branch
from
32401e3
to
51a6c68
Compare
|
An alternative idea would be to add an Pros of doing that would be cleaner anchors, con is that there is an extra attribute. 🤔 |
|
Cool work here Ari! 💯
I don't think the problem lies to the prefix used. Is more about not 'allowing' to edit the generated anchor depending on where you're typing and this can be confusing. When I place the anchor after the prefix in my gif, I'm typing things that seem like it does nothing, but in fact it triggers updates based on the heading's content.
I don't believe adding a new prop like |
|
@ntsekouras I refactored the PR a bit... Now it doesn't use a prefix, and the issue you pointed-out earlier was resolved. |
There was a problem hiding this comment.
Thanks Ari for your work here!
I tried to find a solution without using a fake attribute here: #31174, but isn't working well and I've ping a couple of folks to chime in..
Other than the proper solution for the updates, I noticed that there is a problem also in the auto generated anchor when we have non-latin characters. For example in Καλημέρα κόσμε is autogenerated properly the first time, but if you add latin content it keeps only the latin one..
Also when we find the proper solution, a debounced call should probably be better for this when content changes. But let's make it work well for start. I wish I could be more helpful right now..
That was by design. However, I can see how that would be considered an "error"... I pushed a fix. |
This comment has been minimized.
This comment has been minimized.
|
Pushed another fix for this PR: |
|
Brought the branch up to speed with trunk 👍 |
|
@aristath Any more insight into the block? |
| @@ -19,6 +20,9 @@ import { | |||
| * Internal dependencies | |||
| */ | |||
| import HeadingLevelDropdown from './heading-level-dropdown'; | |||
| import { generateAnchor } from './autogenerate-anchors'; | |||
|
|
|||
| const allHeadingAnchors = {}; | |||
There was a problem hiding this comment.
Nit: I think this is an implementation detail and should be part of the autogenerate-anchors file.
| anchor: generateAnchor( clientId, content, allHeadingAnchors ), | ||
| } ); | ||
| } | ||
| allHeadingAnchors[ clientId ] = anchor; |
There was a problem hiding this comment.
| allHeadingAnchors[ clientId ] = anchor; | |
| setAnchor( clientId, anchor ); |
If you decide to move the cache variable will need a new setAnchor method.
|
Thank you for the review @Mamaduka! Since there are no issues with this implementation I'll go ahead and merge it so we can get things rolling. |
|
@aristath sounds good. I'm also happy to do the follow-up PR if they look okay to you. |
Of course! |
|
Nice! |
|
This PR is not the reason why the TOC block was disabled, at the time there were more serious concerns about the way that block was implemented. |
|
This has a bug where if you write a heading and then duplicate that block the old id is retained even if you change the text for the second heading. It also doesn't take into consideration ids from different editor settings (such as widgets) so you can end up with duplicate ids on the page from the main editor and widgets. I'm also seeing no flag to disable the functionality either. Not speaking personally as its bad practice these days, but I can see this having negative consequences on websites where they use ids for JavaScript hooks or styling where these autogenerated ids have the potential to break the page. |
|
I think there's a case for considering a prefix for these auto-generated anchors. |
Fixes #29561
Description
This PR auto-generates anchors for headings.
It is one of the prerequisites for the table-of-contents block and discussed in #29561
How anchors are generated
wp--1,-2etc suffixwp-prefix is automatically removed.How has this been tested?
-1,-2etc suffix.Καλημέρα κόσμε(roughly translates to hello world).Checklist:
*.native.jsfiles for terms that need renaming or removal).Code for this implementation was ported and adapted from Yoast SEO Premium, with the blessings of @jdevalk