-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Document outline: Use links not buttons #10815
Merged
Merged
Changes from all commits
Commits
Show all changes
81 commits
Select commit
Hold shift + click to select a range
1ccfe6f
Adjust document outline to use an a tag vs button
474535f
target href links directly to page anchors, remove onClick handler
e574784
Merge branch 'master' of github.com:WordPress/gutenberg
63f4800
Merge branch 'master' into fix/doc-outline
e62566d
Merge branch 'master' of github.com:WordPress/gutenberg
ade47e5
Merge branch 'master' into fix/doc-outline
da9311c
update test snapshot
8e54535
Merge branch 'master' of github.com:WordPress/gutenberg
2343fa7
update snapshot
ef96b90
better titleNode targeting
3c2a500
update snapshot
f579dda
update snapshot
acde4a5
Merge branch 'master' of github.com:WordPress/gutenberg
20691f3
Merge branch 'master' of github.com:WordPress/gutenberg
89660bc
Merge branch 'master' of github.com:WordPress/gutenberg
7aee351
Merge branch 'master' into fix/doc-outline
e434bae
Close the table of contents panel when a link is clicked
841bf0f
add deterministic block id to tests
9082baa
remove redundant screen reader text
b64b2ed
Adjust map to avoid mutating original object
e4cc7cf
update snapshot
55b69f5
Update packages/editor/src/components/document-outline/index.js
tofumatt 46e4e1f
Update packages/editor/src/components/document-outline/test/index.js
tofumatt 0aaaaa0
Update packages/editor/src/components/document-outline/index.js
tofumatt 4f85e47
Merge branch 'master' into fix/doc-outline
273c7b4
update snapshot
c69dcf7
Merge branch 'fix/doc-outline' of github.com:WordPress/gutenberg into…
8b576c2
update snapshots
275f225
Merge branch 'master' into fix/doc-outline
eab1548
update snapshot
6df81db
update snapshots
4f38edc
fix up e2e tests
e8aa8d2
Fix snapshots by removing single quotes from outline links
mcsf 33db8e9
Update packages/editor/src/components/document-outline/index.js
mcsf 7d7edf3
Merge branch 'fix/doc-outline' of github.com:WordPress/gutenberg into…
b85b4c9
change target to href
f0afad5
rename close -> closeOutline
06b3cdd
Merge branch 'master' of github.com:WordPress/gutenberg
a97e293
Merge branch 'master' of github.com:WordPress/gutenberg
7091d06
Merge branch 'master' into fix/doc-outline
dd3f8b3
update snapshots after property name changes
69f94c9
rename close/onClose -> onRequestClose for TOC, on
7d3f4a9
restore onSelect
48764d5
complete renaming
e88aac5
Block links are only valid for the current session - remove hash afte…
6a6d8d2
update snapshot
ca3e4be
Merge branch 'master' of github.com:WordPress/gutenberg
d227dfb
Merge branch 'master' of github.com:WordPress/gutenberg
cb94e71
Merge branch 'master' of github.com:WordPress/gutenberg
158bc4a
Merge branch 'master' into fix/doc-outline
0475978
cleanup; move block id hash removal functionality up to document outl…
3604126
update snapshot
729037d
Merge branch 'master' of github.com:WordPress/gutenberg
7e5be38
Merge branch 'master' into fix/doc-outline
bada58f
use replaceState vs pushState
44d6d40
use defer and import at top of file
d5470e5
removeURLHash as helper
849949c
Merge branch 'master' of github.com:WordPress/gutenberg
0a534f6
Merge branch 'master' into fix/doc-outline
2f1b5e2
Merge branch 'master' of github.com:WordPress/gutenberg
2a6aacf
Merge branch 'master' into fix/doc-outline
180cd54
Merge branch 'master' of github.com:WordPress/gutenberg
7c40fd4
Merge branch 'master' into fix/doc-outline
cd26b45
Merge branch 'master' into fix/doc-outline
613bc47
remove passing event in select handler
99380f2
improve doc block
800787e
Merge branch 'master' of github.com:WordPress/gutenberg
506ad72
Merge branch 'master' into fix/doc-outline
9862909
Merge branch 'master' of github.com:WordPress/gutenberg
00e3b35
Merge branch 'master' into fix/doc-outline
ac4d147
Merge branch 'master' into fix/doc-outline
ca63c58
Skip title in outline when title node not found
291097c
remove removeURLHash
da3de52
Merge branch 'master' of github.com:WordPress/gutenberg
d3443e7
Merge branch 'master' of github.com:WordPress/gutenberg
0c7d6b5
Merge branch 'master' of github.com:WordPress/gutenberg
38ca0fe
Merge branch 'master' into fix/doc-outline
03fc448
Merge branch 'master' of github.com:WordPress/gutenberg
bdd533f
Merge branch 'master' into fix/doc-outline
de595d3
Merge branch 'master' of github.com:WordPress/gutenberg
a04038f
Merge branch 'master' into fix/doc-outline
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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.
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.
Is there no selector to get the title? This seems quite brittle.
I wouldn't advocate for this staying, but if it does it needs an E2E test to make sure changing that selector won't regress things.
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 was the only way I could 'discover' the selector though I agree it is brittle. Maybe @aduth has another suggestion how we could do that here.
Otherwise I'll try to add an e2e test.
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.
re: adding e2e tests for this selector:
Reviewing the e2e testing suite, i see numerous existing instances where we rely on the
.editor-post-title__input
selector ( see https://github.com/WordPress/gutenberg/search?q=.editor-post-title__input&unscoped_q=.editor-post-title__input) - if this selector didn't exist all these tests would fail.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 generally fine with this in that it's at least what had existed previously. The two downsides I see with the changes proposed here are:
id
associated with it, which did not exist previously.These are generally minor, and are "par for the course" in updating the implementation to leverage
id
as the anchor point.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.
@aduth @tofumatt are you ok with the current state of this PR? Are there any additional points I can work to address?
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.
One thing I might like to see: The previous implementation guarded against the (real) possibility that the title node could not be found. Here we've removed that protection, which could crash the editor. It could be worth considering to include this in the derived value for
hasTitle
, which would prevent the rendering of theDocumentOutlineItem
if the title node does not exist.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 was never addressed?
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.
Addressed in 9ca6dea