-
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
Changes from 32 commits
1ccfe6f
474535f
e574784
63f4800
e62566d
ade47e5
da9311c
8e54535
2343fa7
ef96b90
3c2a500
f579dda
acde4a5
20691f3
89660bc
7aee351
e434bae
841bf0f
9082baa
b64b2ed
e4cc7cf
55b69f5
46e4e1f
0aaaaa0
4f85e47
273c7b4
c69dcf7
8b576c2
275f225
eab1548
6df81db
4f38edc
e8aa8d2
33db8e9
7d7edf3
b85b4c9
f0afad5
06b3cdd
a97e293
7091d06
dd3f8b3
69f94c9
7d3f4a9
48764d5
e88aac5
6a6d8d2
ca3e4be
d227dfb
cb94e71
158bc4a
0475978
3604126
729037d
7e5be38
bada58f
44d6d40
d5470e5
849949c
0a534f6
2f1b5e2
2a6aacf
180cd54
7c40fd4
cd26b45
613bc47
99380f2
800787e
506ad72
9862909
00e3b35
ac4d147
ca63c58
291097c
da3de52
d3443e7
0c7d6b5
38ca0fe
03fc448
bdd533f
de595d3
a04038f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
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, withDispatch } from '@wordpress/data'; | ||
import { withSelect } from '@wordpress/data'; | ||
import { | ||
create, | ||
getTextContent, | ||
|
@@ -64,7 +64,7 @@ const computeOutlineHeadings = ( blocks = [], path = [] ) => { | |
|
||
const isEmptyHeading = ( heading ) => ! heading.attributes.content || heading.attributes.content.length === 0; | ||
|
||
export const DocumentOutline = ( { blocks = [], title, onSelect, isTitleSupported } ) => { | ||
export const DocumentOutline = ( { blocks = [], title, close, isTitleSupported } ) => { | ||
const headings = computeOutlineHeadings( blocks ); | ||
|
||
if ( headings.length < 1 ) { | ||
|
@@ -73,17 +73,8 @@ export const DocumentOutline = ( { blocks = [], title, onSelect, isTitleSupporte | |
|
||
let prevHeadingLevel = 1; | ||
|
||
// Select the corresponding block in the main editor | ||
// when clicking on a heading item from the list. | ||
const onSelectHeading = ( clientId ) => onSelect( clientId ); | ||
const focusTitle = () => { | ||
// Not great but it's the simplest way to focus the title right now. | ||
const titleNode = document.querySelector( '.editor-post-title__input' ); | ||
if ( titleNode ) { | ||
titleNode.focus(); | ||
} | ||
}; | ||
|
||
// 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. Choose a reason for hiding this commentThe 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 commentThe 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 commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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:
These are generally minor, and are "par for the course" in updating the implementation to leverage There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. Addressed in 9ca6dea |
||
const hasTitle = isTitleSupported && title; | ||
const countByLevel = countBy( headings, 'level' ); | ||
const hasMultipleH1 = countByLevel[ 1 ] > 1; | ||
|
@@ -95,7 +86,8 @@ export const DocumentOutline = ( { blocks = [], title, onSelect, isTitleSupporte | |
<DocumentOutlineItem | ||
level={ __( 'Title' ) } | ||
isValid | ||
onClick={ focusTitle } | ||
close={ close } | ||
target={ `#${ titleNode.id }` } | ||
> | ||
{ title } | ||
</DocumentOutlineItem> | ||
|
@@ -118,8 +110,9 @@ export const DocumentOutline = ( { blocks = [], title, onSelect, isTitleSupporte | |
key={ index } | ||
level={ `H${ item.level }` } | ||
isValid={ isValid } | ||
onClick={ () => onSelectHeading( item.clientId ) } | ||
path={ item.path } | ||
close={ close } | ||
target={ `'#block-${ item.clientId }` } | ||
adamsilverstein marked this conversation as resolved.
Show resolved
Hide resolved
|
||
> | ||
{ item.isEmpty ? | ||
emptyHeadingContent : | ||
|
@@ -149,11 +142,5 @@ export default compose( | |
blocks: getBlocks(), | ||
isTitleSupported: get( postType, [ 'supports', 'title' ], false ), | ||
}; | ||
} ), | ||
withDispatch( ( dispatch ) => { | ||
const { selectBlock } = dispatch( 'core/editor' ); | ||
return { | ||
onSelect: selectBlock, | ||
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'; | ||
|
||
/** | ||
* WordPress dependencies | ||
*/ | ||
import { __ } from '@wordpress/i18n'; | ||
|
||
/** | ||
* Internal dependencies | ||
*/ | ||
|
@@ -17,8 +12,9 @@ const TableOfContentsItem = ( { | |
children, | ||
isValid, | ||
level, | ||
onClick, | ||
path = [], | ||
target, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why is this named target instead of just There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ah, good point - i'll change this to href There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. changed in b85b4c9 |
||
close, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same question as above? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. same answer as above, should I rename this? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. renamed this prop to |
||
} ) => ( | ||
<li | ||
className={ classnames( | ||
|
@@ -29,9 +25,10 @@ const TableOfContentsItem = ( { | |
} | ||
) } | ||
> | ||
<button | ||
<a | ||
href={ target } | ||
className="document-outline__button" | ||
onClick={ onClick } | ||
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> | ||
<span className="screen-reader-text">{ __( '(Click to focus this heading)' ) }</span> | ||
adamsilverstein marked this conversation as resolved.
Show resolved
Hide resolved
|
||
</button> | ||
</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
close
prop mean in the context of aDocumentOutline
component.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
onClick
orcloseOutline
?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
onSelect
that 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