Document outline: Use links not buttons #10815
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -8,7 +8,7 @@ import { countBy, flatMap, get } from 'lodash'; | |
| */ | ||
| import { __ } from '@wordpress/i18n'; | ||
| import { compose } from '@wordpress/compose'; | ||
| import { withSelect } from '@wordpress/data'; | ||
|
|
||
| /** | ||
| * Internal dependencies | ||
|
|
@@ -61,7 +61,7 @@ const computeOutlineHeadings = ( blocks = [], path = [] ) => { | |
|
|
||
| const isEmptyHeading = ( heading ) => ! heading.attributes.content || heading.attributes.content.length === 0; | ||
|
|
||
| export const DocumentOutline = ( { blocks = [], title, close, isTitleSupported } ) => { | ||
| const headings = computeOutlineHeadings( blocks ); | ||
|
|
||
| if ( headings.length < 1 ) { | ||
|
|
@@ -70,17 +70,8 @@ export const DocumentOutline = ( { blocks = [], title, onSelect, isTitleSupporte | |
|
|
||
| let prevHeadingLevel = 1; | ||
|
|
||
| // Not great but it's the simplest way to locate the title right now. | ||
| const titleNode = document.querySelector( '.editor-post-title__input' ); | ||
|
There was a problem hiding this comment. 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. 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. re: adding e2e tests for this selector: Reviewing the e2e testing suite, i see numerous existing instances where we rely on the There was a problem hiding this comment. 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:
These are generally minor, and are "par for the course" in updating the implementation to leverage There was a problem hiding this comment. 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 There was a problem hiding this comment.
This was never addressed? There was a problem hiding this comment. Addressed in 9ca6dea |
||
| const hasTitle = isTitleSupported && title; | ||
| const countByLevel = countBy( headings, 'level' ); | ||
| const hasMultipleH1 = countByLevel[ 1 ] > 1; | ||
|
|
@@ -92,7 +83,8 @@ export const DocumentOutline = ( { blocks = [], title, onSelect, isTitleSupporte | |
| <DocumentOutlineItem | ||
| level={ __( 'Title' ) } | ||
| isValid | ||
| close={ close } | ||
| target={ '#' + titleNode.id } | ||
adamsilverstein marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| > | ||
| { title } | ||
| </DocumentOutlineItem> | ||
|
|
@@ -115,8 +107,9 @@ export const DocumentOutline = ( { blocks = [], title, onSelect, isTitleSupporte | |
| key={ index } | ||
| level={ `H${ item.level }` } | ||
| isValid={ isValid } | ||
| path={ item.path } | ||
| close={ close } | ||
| target={ '#block-' + item.clientId } | ||
adamsilverstein marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| > | ||
| { item.isEmpty ? | ||
| emptyHeadingContent : | ||
|
|
@@ -147,11 +140,5 @@ export default compose( | |
| blocks: getBlocks(), | ||
| isTitleSupported: get( postType, [ 'supports', 'title' ], false ), | ||
| }; | ||
adamsilverstein marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| } ) | ||
| )( DocumentOutline ); | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,11 +3,6 @@ | |
| */ | ||
| import classnames from 'classnames'; | ||
|
|
||
| /** | ||
| * Internal dependencies | ||
| */ | ||
|
|
@@ -17,8 +12,9 @@ const TableOfContentsItem = ( { | |
| children, | ||
| isValid, | ||
| level, | ||
| path = [], | ||
| target, | ||
|
There was a problem hiding this comment. Why is this named target instead of just There was a problem hiding this comment. ah, good point - i'll change this to href There was a problem hiding this comment. changed in b85b4c9 |
||
| close, | ||
|
There was a problem hiding this comment. Same question as above? There was a problem hiding this comment. same answer as above, should I rename this? There was a problem hiding this comment. renamed this prop to |
||
| } ) => ( | ||
| <li | ||
| className={ classnames( | ||
|
|
@@ -29,9 +25,10 @@ const TableOfContentsItem = ( { | |
| } | ||
| ) } | ||
| > | ||
| <a | ||
| href={ target } | ||
| className="document-outline__button" | ||
| onClick={ close } | ||
| > | ||
| <span className="document-outline__emdash" aria-hidden="true"></span> | ||
| { | ||
|
|
@@ -49,8 +46,7 @@ const TableOfContentsItem = ( { | |
| <span className="document-outline__item-content"> | ||
| { children } | ||
| </span> | ||
adamsilverstein marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| </a> | ||
| </li> | ||
| ); | ||
|
|
||
|
|
||
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.
Why this prop was changed. What does a
closeprop mean in the context of aDocumentOutlinecomponent.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 passing through the close functionality so we can close the outline when links are clicked, see https://github.com/WordPress/gutenberg/pull/10815/files#diff-486ea059bfd8ab47600431d73ad9824dR89 - is your question around the naming, eg should this be better named to reflect the functionality - eg called
onClickorcloseOutline?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 the question is specifically: What was wrong with
onSelectthat it required changing?There's a bit of conconvention in the naming introduced here, much more imperative "close" than event-driven "onClose" that had existed previously / exists elsewhere.
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.
ah, ok - thanks for clarifying. Still getting used to the conventions here, I'll change that back to
onSelect