Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add Table of Contents block #21040
Changes from 6 commits
e5bea61
ba40205
ba5c43a
8507968
50df5b5
4c650b5
371a901
d2e929b
c0f2e1a
4fce9a1
cb3e495
bcfc383
f6aeb36
366d7bd
0648776
29e3c05
5cfade2
b89368d
b82a9e5
7e4a236
10d49be
32a376a
e49f28d
becb1a9
e7f3dae
8cbe8fc
36adc5b
45ee5f7
04e0df2
afacffc
796900a
d31534c
6d1d82c
a96a5dc
00255a0
af0f900
9c73d7b
99bbe0b
d9037e1
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
There are a lot of separate calls to
getPageHeadings
: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.
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.
I got rid of
refresh
altogether - it was left over from some removed code. NowgetPageHeadings
(now calledgetHeadingsList
) is only used incomponentDidMount
.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.
Looking at the implementation of
getPageHeadings
, it returns a new array after mappinggetBlocks
every time and that's passed tosetState
, 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.
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.
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.
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.
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.
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.
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.
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.
I'm not completely sure about the need to use
componentDidUpdate
, it seems like this could go in thesubscribe
callback. It's good to reduce component updates/renders, and it seems like usingcomponentDidUpdate
will result in two successive renders.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 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?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.
I think this description needs to be updated since the block no longer automatically adds anchors to the headings.
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.
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)?
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.
I'd say "headings" rather than "headers". Also, the contributor guide recommends avoiding describing features as "allowing" something.
How's this?
You might be able to leave off "automatically".
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.
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.
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.
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.