-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Post author dropdown: add accessible-autocomplete #7385
Changes from 110 commits
ea01c26
f84c371
240c304
0d1991d
ea094fb
642ae6f
4ab2777
3ebc2c2
a526c60
a8e9bb1
aad6dcb
16dd20c
b056747
6e69cf0
12e58b0
14edb8a
6d555c7
853dfe6
83a7be4
940f338
18290b2
42c6a60
1e322aa
8da09fc
e31b540
7a70911
7e7696b
6c765c7
1b10c63
7aab164
4e47a3f
03e0155
3e33275
6238e4c
9d1b490
efac333
7ec4b6c
8a9e711
5173a7a
e4915b0
86b3d31
d6e1deb
6bdba75
a683a1c
46d27f9
c3bf43d
877032a
9e42571
43ccd6c
79b6c66
708fd55
aa5376f
4f94442
1ecfa05
c6a9b18
06b3cdd
319c96c
a97e293
839a618
550c63a
d1731ec
6585f70
ca3e4be
66daf9f
74e4083
c70e125
cdf7bde
4aded2f
f1b5739
d227dfb
205f07a
790c2c2
cb94e71
729037d
849949c
91a891d
2f1b5e2
76090f7
e6a8713
74e4f71
ac7bedd
783655c
180cd54
e45ffe0
525cf40
93b5396
9a35172
31d50b5
50400fc
ac21e7b
800787e
a021841
3fdd917
741a2a8
9862909
96ed175
1980080
624c49d
fcaf60d
da3de52
d3443e7
1217076
1d13ad8
8d3ae5b
db3bea3
807b61e
5ba93d1
0c7d6b5
03fc448
aa4bbcc
b3839b3
e593595
d528303
27cedb3
7e5d82d
4c9fd8b
4d8c249
b830022
2426493
bd6ef21
0b12bfd
28bf036
2daaa8c
1e824ad
c89d301
8d7db70
43d4a59
4c6a447
83f6086
3317851
615e3d9
cbea8ff
0066508
ba26d03
eb64f3b
24ddea3
92e4074
71b7243
73343ae
e445aa7
6e129da
e545898
1c1fb89
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,10 +1,17 @@ | ||
/** | ||
* External dependencies | ||
*/ | ||
import Autocomplete from 'accessible-autocomplete/react'; | ||
import { debounce } from 'lodash'; | ||
|
||
/** | ||
* WordPress dependencies | ||
*/ | ||
import { __ } from '@wordpress/i18n'; | ||
import { sprintf, __, _n } from '@wordpress/i18n'; | ||
import { withInstanceId, compose } from '@wordpress/compose'; | ||
import { Component } from '@wordpress/element'; | ||
import { withSelect, withDispatch } from '@wordpress/data'; | ||
import apiFetch from '@wordpress/api-fetch'; | ||
|
||
/** | ||
* Internal dependencies | ||
|
@@ -16,34 +23,145 @@ export class PostAuthor extends Component { | |
super( ...arguments ); | ||
|
||
this.setAuthorId = this.setAuthorId.bind( this ); | ||
this.suggestAuthor = this.suggestAuthor.bind( this ); | ||
this.getCurrentAuthor = this.getCurrentAuthor.bind( this ); | ||
this.resolveResults = this.resolveResults.bind( this ); | ||
noisysocks marked this conversation as resolved.
Show resolved
Hide resolved
|
||
this.requestResults = debounce( ( query, populateResults ) => { | ||
const payload = '?search=' + encodeURIComponent( query ); | ||
apiFetch( { path: `/wp/v2/users${ payload }` } ).then( ( results ) => { | ||
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. Ideally a component should never be calling 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. Ok, I'll switch this to using core/data adding whatever support we need there.
will core/data account for that or should i be endeavouring to cancel these requests when the component unmounts? another option would be tracking mount state and doing ending silently for request when unmounted. 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.
It's a non-concern for you. In practice, it doesn't actually cancel it, but it doesn't really matter, and also helps if a future component mount also needs the same data (another reason why instance-specific fetches should be discouraged, since the data can / should be shared). 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 see we also use 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. @aduth Thinking about adding the capability to select both a single user by ID and to search users via the data component - currently this does not seem possible. Do you think I should add this to the existing 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. @aduth any feedback on this question? 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's some precedent here with the existing "entities", currently post types, taxonomies, and media items. The entities implementation will auto-generate two selectors:
Considering
And, in fact, it could be both, where the first is just an abstraction on the second. I'm not sure if I've missed anything, but I'd rather not have |
||
populateResults( this.resolveResults( results ) ); | ||
this.searchCache[ query ] = results; | ||
} ); | ||
}, 300 ); | ||
this.searchCache = []; | ||
this.state = { | ||
postAuthor: false, | ||
}; | ||
} | ||
|
||
getInitialPostAuthor() { | ||
// Get the initial author from props if available. | ||
return this.props.postAuthor; | ||
} | ||
|
||
componentDidMount() { | ||
const { postAuthorId, authors } = this.props; | ||
|
||
// Load the initial post author. | ||
const postAuthor = this.getInitialPostAuthor(); | ||
this.setState( { postAuthor } ); | ||
|
||
const authorInAuthors = authors.find( ( singleAuthor ) => { | ||
return singleAuthor.id === postAuthorId; | ||
} ); | ||
|
||
// Set or fetch the current author. | ||
if ( authorInAuthors ) { | ||
this.setState( { postAuthor: authorInAuthors } ); | ||
} else { | ||
this.getCurrentAuthor( postAuthorId ); | ||
} | ||
} | ||
|
||
suggestAuthor( query, populateResults ) { | ||
if ( query.length < 2 ) { | ||
return; | ||
} | ||
|
||
if ( this.searchCache[ query ] ) { | ||
populateResults( this.resolveResults( this.searchCache[ query ] ) ); | ||
return; | ||
} | ||
|
||
this.requestResults( query, populateResults ); | ||
} | ||
|
||
setAuthorId( event ) { | ||
resolveResults( results ) { | ||
this.authors = results; | ||
return results.map( ( author ) => ( author.name ) ); | ||
} | ||
|
||
getCurrentAuthor( authorId ) { | ||
if ( ! authorId ) { | ||
return; | ||
} | ||
apiFetch( { path: `/wp/v2/users/${ encodeURIComponent( authorId ) }` } ).then( ( results ) => { | ||
this.setState( { postAuthor: results } ); | ||
} ); | ||
} | ||
|
||
setAuthorId( selection ) { | ||
if ( ! selection ) { | ||
return; | ||
} | ||
const { onUpdateAuthor } = this.props; | ||
const { value } = event.target; | ||
onUpdateAuthor( Number( value ) ); | ||
if ( typeof selection === 'string' ) { | ||
// Author name from the autocompleter. | ||
const author = this.authors.find( ( singleAuthor ) => { | ||
return singleAuthor.name === selection; | ||
} ); | ||
onUpdateAuthor( Number( author.id ) ); | ||
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. Do we have a guarantee that 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. Yes, I think we do. This handler gets the selection, which the author name, then searches the list of options (of which the selection is one) to extract the id. one potential issue though would be duplicate names. I'll try to change this to pass the id instead. 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.
Did this happen? 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. Digging into this a bit, the current implementation does not support passing an id to the searching function - only the value. This made me concerned about duplicate user names which led me to reviewing the current ("classic editor") implementation which I re-discovered includes the (unique) user slug as part of the select values: I updated the PR here to match, guaranteeing unique author 'names' - actually "Name (slug)". In addition, I added a check to ensure on a successful author selection would be set. |
||
} else { | ||
// Author ID from the select. | ||
onUpdateAuthor( Number( selection.target.value ) ); | ||
} | ||
} | ||
|
||
render() { | ||
const { postAuthor, instanceId, authors } = this.props; | ||
const selectId = 'post-author-selector-' + instanceId; | ||
|
||
// Disable reason: A select with an onchange throws a warning | ||
adamsilverstein marked this conversation as resolved.
Show resolved
Hide resolved
|
||
const { postAuthor } = this.state; | ||
const { postAuthorId, instanceId, authors } = this.props; | ||
const selectId = `post-author-selector-${ instanceId }`; | ||
let selector; | ||
if ( ! postAuthor ) { | ||
return null; | ||
} | ||
|
||
/* eslint-disable jsx-a11y/no-onchange */ | ||
return ( | ||
<PostAuthorCheck> | ||
<label htmlFor={ selectId }>{ __( 'Author' ) }</label> | ||
if ( authors.length < 50 ) { | ||
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. Can you refer me to discussion where it was decided to use a different select input depending on the number of users? It gives me pause because it introduces unpredictability, an extra maintenance burden, and throws some doubt into the general usability of the autocomplete component (why isn't it good enough to use for a site with only a few users?). 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. Thanks @swissspidy . That leads to a recommendation, but it isn't very clear on the specific reasons around why it would be a good idea to change the behavior depending on the number of options. The specific phrasing reads a bit confused to me as well, being ambiguous on whether or not "you can use 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.
It's a good idea because with a small number of options, a regular HTML select element is an excellent choice: its concise, accessible, and a fundamental HTML component that all browsers and accessibility aids such as screen readers handle well. Beyond a certain size, however, dropdowns become a poor choice: they are unwieldy, all elements must be loaded ahead of time before the dropdown can display resulting in a slow load time especially on low bandwidth connections, searching by typing is difficult (pausing often breaks the search), very large dropdowns crash the browser on less powerful devices, searching large dropdowns on mobile is nearly impossible, etc etc.
Right, this is not an accessibility issue directly. Rather it is a bandwidth, memory, best practice, usability, scalability etc issue. Regarding accessibility: the technical solution I have chosen to address the many issues with a large dropdown is accessible. 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. Thanks for the explanation. As long as we're acknowledging the following, I think it may be an agreeable compromise:
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. Right.
AccessibleAutocomplete explicitly recommends this type of progressive enhancement: native elements work very well for smaller numbers of elements.
Yes, we are accepting maintaining the handling of both states (the implementations themselves are well maintained elsewhere). 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'd totally second this. The autocomplete "filtering" feature doesn't make sense when the amount of options is limited. As a user, I really don't need to filter a list if I can see all the available (few) options. It does help a lot when there are lots of options. |
||
// Disable reason: A select with an onchange throws a warning. | ||
/* eslint-disable jsx-a11y/no-onchange */ | ||
selector = | ||
<select | ||
id={ selectId } | ||
value={ postAuthor } | ||
onChange={ this.setAuthorId } | ||
value={ postAuthorId } | ||
className="editor-post-author__select" | ||
onChange={ this.setAuthorId } | ||
> | ||
{ authors.map( ( author ) => ( | ||
<option key={ author.id } value={ author.id }>{ author.name }</option> | ||
) ) } | ||
</select> | ||
</select>; | ||
} else { | ||
selector = | ||
<Autocomplete | ||
id={ selectId } | ||
minLength={ 2 } | ||
showAllValues={ true } | ||
defaultValue={ postAuthor ? postAuthor.name : '' } | ||
displayMenu="overlay" | ||
onConfirm={ this.setAuthorId } | ||
source={ this.suggestAuthor } | ||
showNoResultsFound={ false } | ||
tStatusQueryTooShort={ ( minQueryLength ) => | ||
// translators: %d: the number characters required to initiate an author search. | ||
sprintf( __( 'Type in %d or more characters for results' ), minQueryLength ) | ||
} | ||
tStatusNoResults={ () => __( 'No search results' ) } | ||
tStatusSelectedOption={ ( selectedOption, length ) => __( `${ selectedOption } (1 of ${ length }) is selected` ) } | ||
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 believe this will throw an 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. |
||
tStatusResults={ ( length, contentSelectedOption ) => { | ||
return ( | ||
_n( '%d result is available.', '%d results are available.', length ) + | ||
' ' + contentSelectedOption | ||
); | ||
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. As mentioned in the twin PR #16666 (review) I guess there should be a |
||
} } | ||
cssNamespace="components-post-author__autocomplete" | ||
|
||
/>; | ||
} | ||
|
||
return ( | ||
<PostAuthorCheck> | ||
<label htmlFor={ selectId }>{ __( 'Author' ) }</label> | ||
{ selector } | ||
</PostAuthorCheck> | ||
); | ||
/* eslint-enable jsx-a11y/no-onchange */ | ||
|
@@ -53,7 +171,7 @@ export class PostAuthor extends Component { | |
export default compose( [ | ||
withSelect( ( select ) => { | ||
return { | ||
postAuthor: select( 'core/editor' ).getEditedPostAttribute( 'author' ), | ||
postAuthorId: select( 'core/editor' ).getEditedPostAttribute( 'author' ), | ||
authors: select( 'core' ).getAuthors(), | ||
}; | ||
} ), | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,94 @@ | ||
.components-post-author__autocomplete__wrapper { | ||
position: relative; | ||
} | ||
|
||
.components-post-author__autocomplete__hint, | ||
.components-post-author__autocomplete__input { | ||
-webkit-appearance: none; | ||
border: 2px solid; | ||
border-radius: 0; /* Safari 10 on iOS adds implicit border rounding. */ | ||
box-sizing: border-box; | ||
-moz-box-sizing: border-box; | ||
-webkit-box-sizing: border-box; | ||
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. We use autoprefixer to handle necessary prefixes automatically, so they should be removed from the source files altogether. In any case, |
||
margin-bottom: 0; /* BUG: Safari 10 on macOS seems to add an implicit margin. */ | ||
width: 100%; | ||
} | ||
|
||
.components-post-author__autocomplete__input { | ||
background-color: transparent; | ||
position: relative; | ||
} | ||
|
||
.components-post-author__autocomplete__hint { | ||
position: absolute; | ||
} | ||
|
||
.components-post-author__autocomplete__dropdown-arrow-down { | ||
display: none; | ||
} | ||
|
||
.components-post-author__autocomplete__menu { | ||
background: $white; | ||
border-top: 0; | ||
color: $dark-gray-300; | ||
margin: 0; | ||
padding: #{$grid-size-small / 2} 0; | ||
overflow-x: hidden; | ||
overflow-y: auto; | ||
max-height: 200px; | ||
transition: all 0.15s ease-in-out; | ||
width: calc(100% + 2px); | ||
|
||
&.components-post-author__autocomplete__menu--visible { | ||
display: block; | ||
} | ||
|
||
&.components-post-author__autocomplete__menu--hidden { | ||
display: none; | ||
} | ||
} | ||
|
||
.components-post-author__autocomplete__menu--overlay { | ||
box-shadow: $shadow-popover; | ||
border: $border-width solid $light-gray-500; | ||
left: 0; | ||
position: absolute; | ||
top: 100%; | ||
z-index: 100; | ||
} | ||
|
||
.components-post-author__autocomplete__menu--inline { | ||
position: relative; | ||
} | ||
|
||
.components-post-author__autocomplete__option { | ||
font-size: $default-font-size; | ||
padding: #{$grid-size-small / 2} $grid-size; | ||
margin-bottom: 0; | ||
cursor: pointer; | ||
display: block; | ||
position: relative; | ||
|
||
> * { | ||
pointer-events: none; | ||
} | ||
|
||
&:hover { | ||
background: $light-gray-500; | ||
} | ||
|
||
&:focus, | ||
&.components-post-author__autocomplete__option--focused | ||
&.components-post-author__autocomplete__option--focused:hover { | ||
background: color(theme(primary) shade(15%)); | ||
color: $white; | ||
outline: none; | ||
} | ||
|
||
&.components-post-author__autocomplete__option--no-results, | ||
&.components-post-author__autocomplete__option--no-results:hover { | ||
background: $white; | ||
font-style: italic; | ||
cursor: not-allowed; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -41,14 +41,23 @@ describe( 'PostAuthor', () => { | |
}, | ||
}; | ||
|
||
const postAuthor = [ | ||
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 component has significantly increased in complexity, but its tests do not provide sufficient coverage. 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 am happy to work on additional tests - what do you think would be most useful to test? Thinking e2e tests that verify expected behavior with <50 users and over, I'll review what we have and suggest some additional tests. Worth noting that the autocomplete component itself has 90% test coverage, so we probably don't need to test the component itself - only its integration into Gutenberg. https://github.com/alphagov/accessible-autocomplete 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. A good target is a test case for every single possible combination of behaviors which could occur through the introduced functions / logic flows. Going in with this mindset can be a useful deterrent to complex interactions. 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. Makes sense - I'll work on documenting what those are first 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.
What came of 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.
I bet we only have one or a few users when running e2e tests. We can always add more of them using WP CLI during the setup of the Docker container to speed up things. Otherwise, you would have to write logic which switches to the admin account and is doing the dance to add more users. I'm not sure if this would be performant enough. Ideally, we have a mix of tests where component tests cover this edge cases you listed and e2e tests does some basic interactions testing. Aside: I don't think there should be 2 different components rendered depending on the number of results. @adamsilverstein is it the final proposal? I'm sure you had more PRs exploring alternative solutions. 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. @gziolo Most E2E suites traditionally can be run on the same instance without needing to reset site state. This is why I'm leaning more toward component tests here as we would want to exercise UI pieces in the <50 and >50 user scenario. 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.
Thank you for the offer of help :) Please feel free to make commits directly to the branch! 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 have tried several approaches here. The reasoning behind two different components depending on the number of users is that when the number of options in a dropdown is small, regular HTMl selectors are an excellent choice, are semantically correct and properly accessible as they are browser native. Once the select grows beyond a certain size a auto-completing selector is much more user friendly (and still needs to remain fully accessible). In addition, using two separate components (or rather "progressive enhancement") based on the number of items is the official recommendation from the accessible autocomplete repository, although they do say 'a few hundred' so my choice of 50 as the cutoff may be too low. I'd like to receive more feedback or even perform user testing to validate that. One WordPress specific consideration is that the REST API only returns 100 items max by default, so we may want the cutoff around that to avoid paginated requests on load. See https://github.com/alphagov/accessible-autocomplete#progressive-enhancement 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.
It might a better way to set the limit from the technical perspective. The only question is how likely it is that 100 items in the |
||
{ | ||
id: 1, | ||
name: 'admin', | ||
}, | ||
]; | ||
|
||
describe( '#render()', () => { | ||
it( 'should update author', () => { | ||
const onUpdateAuthor = jest.fn(); | ||
const wrapper = shallow( | ||
<PostAuthor | ||
authors={ authors } | ||
user={ user } | ||
onUpdateAuthor={ onUpdateAuthor } /> | ||
onUpdateAuthor={ onUpdateAuthor } | ||
postAuthor={ postAuthor } | ||
/> | ||
); | ||
|
||
wrapper.find( 'select' ).simulate( 'change', { | ||
|
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 should be defined as a
dependencies
ofpackages/editor/package.json
, not here.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.
moved in 27cedb3