Skip to content
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

Closed
wants to merge 143 commits into from
Closed
Show file tree
Hide file tree
Changes from 110 commits
Commits
Show all changes
143 commits
Select commit Hold shift + click to select a range
ea01c26
Merge branch 'master' of github.com:WordPress/gutenberg
Jun 19, 2018
f84c371
accessible autocomplete, first pass
Jun 19, 2018
240c304
Add apiRequest author search
Jun 19, 2018
0d1991d
debounce search, only show autocomplete when more than 50 users
Jun 19, 2018
ea094fb
Add i18n
Jun 19, 2018
642ae6f
cleanup/whitespace
Jun 19, 2018
4ab2777
Only fetch 100 users in getAuthors
Jun 19, 2018
3ebc2c2
Remove the filter that enabled passing `-1` to get the first 10000 us…
Jun 19, 2018
a526c60
remove an old inline comment that is no longer relevant
Jun 19, 2018
a8e9bb1
Add the accessible-autocomplete npm package
Jun 19, 2018
aad6dcb
whitespace
Jun 19, 2018
16dd20c
manually limit search to strings of 2 or more characters
Jun 19, 2018
b056747
use backticks for template literals
Jun 19, 2018
6e69cf0
fixes for eslint
Jun 19, 2018
12e58b0
Revert "Remove the filter that enabled passing `-1` to get the first …
Jun 20, 2018
14edb8a
Merge branch 'master' into feature/accessible-autocomplete
Jul 4, 2018
6d555c7
use <Autocomplete /> component for more than 50 authors
Jul 4, 2018
853dfe6
Merge branch 'master' of github.com:WordPress/gutenberg
Jul 5, 2018
83a7be4
Merge branch 'master' into feature/accessible-autocomplete
Jul 5, 2018
940f338
Merge branch 'master' of github.com:WordPress/gutenberg
Jul 6, 2018
18290b2
Merge branch 'master' of github.com:WordPress/gutenberg
Jul 6, 2018
42c6a60
accessible autocomplete, first pass
Jun 19, 2018
1e322aa
Add apiRequest author search
Jun 19, 2018
8da09fc
debounce search, only show autocomplete when more than 50 users
Jun 19, 2018
e31b540
Add i18n
Jun 19, 2018
7a70911
cleanup/whitespace
Jun 19, 2018
7e7696b
Only fetch 100 users in getAuthors
Jun 19, 2018
6c765c7
Remove the filter that enabled passing `-1` to get the first 10000 us…
Jun 19, 2018
1b10c63
remove an old inline comment that is no longer relevant
Jun 19, 2018
7aab164
Add the accessible-autocomplete npm package
Jun 19, 2018
4e47a3f
whitespace
Jun 19, 2018
03e0155
manually limit search to strings of 2 or more characters
Jun 19, 2018
3e33275
use backticks for template literals
Jun 19, 2018
6238e4c
fixes for eslint
Jun 19, 2018
9d1b490
Revert "Remove the filter that enabled passing `-1` to get the first …
Jun 20, 2018
efac333
use <Autocomplete /> component for more than 50 authors
Jul 4, 2018
7ec4b6c
Bail early if no postAuthor, add missing semicolon
Jul 9, 2018
8a9e711
Hello eslint!
Jul 9, 2018
5173a7a
Merge branch 'feature/accessible-autocomplete' of github.com:WordPres…
Jul 9, 2018
e4915b0
Merge branch 'master' of github.com:WordPress/gutenberg
Jul 9, 2018
86b3d31
Merge branch 'master' into feature/accessible-autocomplete
Jul 9, 2018
d6e1deb
Merge branch 'master' of github.com:WordPress/gutenberg
Jul 27, 2018
6bdba75
Merge branch 'master' into feature/accessible-autocomplete
Jul 27, 2018
a683a1c
update package-lock
Jul 27, 2018
46d27f9
add css
Jul 27, 2018
c3bf43d
Merge remote-tracking branch 'origin/master' into feature/accessible-…
notnownikki Oct 15, 2018
877032a
Merge branch 'feature/accessible-autocomplete' of github.com:WordPres…
Oct 18, 2018
9e42571
Merge branch 'master' of github.com:WordPress/gutenberg
Oct 18, 2018
43ccd6c
Merge branch 'master' into feature/accessible-autocomplete
Oct 18, 2018
79b6c66
include postAuthor data in PostAuthor test
Oct 18, 2018
708fd55
override some dropdown styles
Oct 18, 2018
aa5376f
Merge branch 'master' into feature/accessible-autocomplete
Oct 28, 2018
4f94442
Merge branch 'master' into feature/accessible-autocomplete
Nov 25, 2018
1ecfa05
Merge branch 'master' into feature/accessible-autocomplete
Dec 3, 2018
c6a9b18
update package lock
Dec 3, 2018
06b3cdd
Merge branch 'master' of github.com:WordPress/gutenberg
Dec 10, 2018
319c96c
Merge branch 'master' into feature/accessible-autocomplete
Dec 10, 2018
a97e293
Merge branch 'master' of github.com:WordPress/gutenberg
Dec 14, 2018
839a618
Merge branch 'master' into feature/accessible-autocomplete
Dec 14, 2018
550c63a
current author load on mount
Dec 17, 2018
d1731ec
Set or fetch the current author
Dec 17, 2018
6585f70
If the postAuthor is provided, use it directly. Fixes test, enables p…
Dec 17, 2018
ca3e4be
Merge branch 'master' of github.com:WordPress/gutenberg
Dec 17, 2018
66daf9f
Merge branch 'master' into feature/accessible-autocomplete
Dec 17, 2018
74e4083
Build docs
swissspidy Dec 18, 2018
c70e125
remove unused getAuthor resolver
Dec 18, 2018
cdf7bde
Merge branch 'feature/accessible-autocomplete' of github.com:WordPres…
Dec 18, 2018
4aded2f
lodash!
Dec 18, 2018
f1b5739
findWhere -> find
Dec 18, 2018
d227dfb
Merge branch 'master' of github.com:WordPress/gutenberg
Dec 19, 2018
205f07a
Merge branch 'master' into feature/accessible-autocomplete
Dec 19, 2018
790c2c2
add caching; only debounce remote requests
Dec 19, 2018
cb94e71
Merge branch 'master' of github.com:WordPress/gutenberg
Dec 21, 2018
729037d
Merge branch 'master' of github.com:WordPress/gutenberg
Jan 4, 2019
849949c
Merge branch 'master' of github.com:WordPress/gutenberg
Jan 10, 2019
91a891d
Merge branch 'master' into feature/accessible-autocomplete
Jan 10, 2019
2f1b5e2
Merge branch 'master' of github.com:WordPress/gutenberg
Jan 12, 2019
76090f7
Merge branch 'master' into feature/accessible-autocomplete
Jan 12, 2019
e6a8713
remove autoprefixer
Jan 12, 2019
74e4f71
use string literals
Jan 12, 2019
ac7bedd
use sprintf
Jan 12, 2019
783655c
use _n
Jan 12, 2019
180cd54
Merge branch 'master' of github.com:WordPress/gutenberg
Jan 18, 2019
e45ffe0
Merge branch 'master' into feature/accessible-autocomplete
Jan 18, 2019
525cf40
remove minified styles, unused
Jan 18, 2019
93b5396
imports in order of increasing locality
Jan 18, 2019
9a35172
remove un-needed bind
Jan 18, 2019
31d50b5
re-add disable reason
Jan 18, 2019
50400fc
Style updates
kjellr Jan 24, 2019
ac21e7b
Cleaning up size variables.
kjellr Jan 24, 2019
800787e
Merge branch 'master' of github.com:WordPress/gutenberg
Jan 25, 2019
a021841
Merge branch 'feature/accessible-autocomplete' of github.com:WordPres…
Jan 25, 2019
3fdd917
Merge branch 'master' into feature/accessible-autocomplete
Jan 25, 2019
741a2a8
Remove some unused styles
kjellr Jan 25, 2019
9862909
Merge branch 'master' of github.com:WordPress/gutenberg
Jan 28, 2019
96ed175
Merge branch 'feature/accessible-autocomplete' of github.com:WordPres…
Jan 28, 2019
1980080
Merge branch 'master' into feature/accessible-autocomplete
Jan 28, 2019
624c49d
rearrange constructor calls
Jan 28, 2019
fcaf60d
getInitialPostAuthor to pull author from props
Jan 28, 2019
da3de52
Merge branch 'master' of github.com:WordPress/gutenberg
Feb 6, 2019
d3443e7
Merge branch 'master' of github.com:WordPress/gutenberg
Feb 10, 2019
1217076
Merge branch 'master' into feature/accessible-autocomplete
Feb 10, 2019
1d13ad8
improve tStatusResults string construction
Feb 10, 2019
8d3ae5b
remove unused postAuthor check
Feb 10, 2019
db3bea3
adjust z-index use & document
Feb 10, 2019
807b61e
prefix css classes
Feb 10, 2019
5ba93d1
remove autoselect
Feb 10, 2019
0c7d6b5
Merge branch 'master' of github.com:WordPress/gutenberg
Feb 20, 2019
03fc448
Merge branch 'master' of github.com:WordPress/gutenberg
Feb 28, 2019
aa4bbcc
Merge branch 'master' into feature/accessible-autocomplete
Feb 28, 2019
b3839b3
Merge branch 'master' of github.com:WordPress/gutenberg
May 16, 2019
e593595
Merge branch 'master' of github.com:WordPress/gutenberg
Jun 23, 2019
d528303
Merge branch 'master' of github.com:WordPress/gutenberg
Jun 25, 2019
27cedb3
move package.json dependency to editor
Jun 25, 2019
7e5d82d
Merge branch 'master' into feature/accessible-autocomplete
Jun 25, 2019
4c9fd8b
update package lock
Jun 25, 2019
4d8c249
increase author cutoff count to 99 and add inline doc explaining
Jun 25, 2019
b830022
correct translation string
Jun 25, 2019
2426493
ensure unique authors; check found before swetting
Jun 25, 2019
bd6ef21
Improve inline documentation
Jun 26, 2019
0b12bfd
helper for author slugs
Jun 26, 2019
28bf036
display a default list when the current selection is unchanged
Jun 26, 2019
2daaa8c
Post Author: add component tests to document when a select or autocom…
gwwar Jun 27, 2019
1e824ad
Merge branch 'master' of github.com:WordPress/gutenberg
Jul 1, 2019
c89d301
Merge branch 'master' of github.com:WordPress/gutenberg
Jul 2, 2019
8d7db70
Merge branch 'feature/accessible-autocomplete' of github.com:WordPres…
Jul 2, 2019
43d4a59
Merge branch 'master' into feature/accessible-autocomplete
Jul 2, 2019
4c6a447
correct typo
Jul 16, 2019
83f6086
correct typo
Jul 16, 2019
3317851
Merge branch 'feature/accessible-autocomplete' of github.com:WordPres…
Aug 27, 2019
615e3d9
Merge branch 'master' into feature/accessible-autocomplete
Aug 27, 2019
cbea8ff
update packacge-lock
Aug 27, 2019
0066508
merge master
adamsilverstein Dec 20, 2019
ba26d03
Merge branch 'master' into feature/accessible-autocomplete
adamsilverstein Dec 23, 2019
eb64f3b
update package lock
adamsilverstein Dec 23, 2019
24ddea3
Upgrade accessible-autocomplete to version 2.0.1.
adamsilverstein Dec 23, 2019
92e4074
correct doc block typo
adamsilverstein Dec 23, 2019
71b7243
Add post author endpoint to preccache via `block_editor_preload_paths`
adamsilverstein Dec 23, 2019
73343ae
Improve auto-complete styling.
adamsilverstein Dec 23, 2019
e445aa7
Get post author object with a getPostAuthor selector
adamsilverstein Dec 23, 2019
6e129da
Show up to 10 items in menu.
adamsilverstein Dec 23, 2019
e545898
Correct construction of tStatusSelectedOption using sprintf.
adamsilverstein Dec 23, 2019
1c1fb89
Add expected_user_path to tests.
adamsilverstein Dec 23, 2019
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions assets/stylesheets/_z-index.scss
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,9 @@ $z-layers: (
".components-drop-zone": 100,
".components-drop-zone__content": 110,

// Autocomplete dropdown shows over other sidebar elements.
".autocomplete__menu--overlay": 100,

// The block mover, particularly in nested contexts,
// should overlap most block content.
".editor-block-list__block.is-{selected,hovered} .editor-block-mover": 80,
Expand Down
15 changes: 15 additions & 0 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,7 @@
"@wordpress/npm-package-json-lint-config": "file:packages/npm-package-json-lint-config",
"@wordpress/postcss-themes": "file:packages/postcss-themes",
"@wordpress/scripts": "file:packages/scripts",
"accessible-autocomplete": "1.6.2",
Copy link
Member

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 of packages/editor/package.json, not here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

moved in 27cedb3

"benchmark": "2.1.4",
"browserslist": "4.4.1",
"chalk": "2.4.1",
Expand Down
2 changes: 1 addition & 1 deletion packages/core-data/src/resolvers.js
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ import { apiFetch } from './controls';
* Requests authors from the REST API.
*/
export function* getAuthors() {
const users = yield apiFetch( { path: '/wp/v2/users/?who=authors&per_page=-1' } );
const users = yield apiFetch( { path: '/wp/v2/users/?who=authors&per_page=100' } );
yield receiveUserQuery( 'authors', users );
}

Expand Down
4 changes: 2 additions & 2 deletions packages/editor/src/components/autocompleters/user.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,9 +15,9 @@ export default {
options( search ) {
let payload = '';
if ( search ) {
payload = '?search=' + encodeURIComponent( search );
payload = `?search=${ encodeURIComponent( search ) }`;
}
return apiFetch( { path: '/wp/v2/users' + payload } );
return apiFetch( { path: `/wp/v2/users${ payload }` } );
},
isDebounced: true,
getOptionKeywords( user ) {
Expand Down
150 changes: 134 additions & 16 deletions packages/editor/src/components/post-author/index.js
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
Expand All @@ -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 ) => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally a component should never be calling apiFetch directly, and rather use the core data resources as the canonical source for REST API entities. One issue with how this is implemented is we have nothing to cancel the behaviors from taking place if the component suddenly becomes unmounted, potentially leading to errors.

Copy link
Member Author

Choose a reason for hiding this comment

The 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.

we have nothing to cancel the behaviors from taking place

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will core/data account for that or should i be endeavouring to cancel these requests when the component unmounts?

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).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see we also use apiFetch in the user autocompleter - https://github.com/WordPress/gutenberg/blob/master/packages/editor/src/components/autocompleters/user.js#L20 - i think to replace thse we'll need a new resolver, perhaps searchAuthors?

Copy link
Member Author

Choose a reason for hiding this comment

The 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 getAuthors selector, eg the ability to pass an id or search query to the selector? or create something new?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@aduth any feedback on this question?

Copy link
Member

Choose a reason for hiding this comment

The 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 getAuthors selector, eg the ability to pass an id or search query to the selector? or create something new?

There's some precedent here with the existing "entities", currently post types, taxonomies, and media items. The entities implementation will auto-generate two selectors:

  • One for retrieving a singular data entry, accepting an ID as its sole argument
  • Another for querying data, accepting the REST API query as its argument

Considering getAuthors as a sort of partially-applied entities selector, I could see this being applied as either:

  • Following the above in spirit, accepting a query object for getAuthors and creating a new getAuthor selector
  • Implementing the users entity explicitly, where querying authors just becomes part of the query object passed to getUsers

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 getAuthors diverge as being its own special thing if it's possible for us to bring it in line with these other standard entities behaviors.

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 ) );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we have a guarantee that author is not undefined here from the result of the prior Array#find? Otherwise this will throw an error.

Copy link
Member Author

Choose a reason for hiding this comment

The 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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll try to change this to pass the id instead.

Did this happen?

Copy link
Member Author

@adamsilverstein adamsilverstein Jun 25, 2019

Choose a reason for hiding this comment

The 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 ) {
Copy link
Member

Choose a reason for hiding this comment

The 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?).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

@aduth aduth Feb 8, 2019

Choose a reason for hiding this comment

The 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 enhanceSelectElement to enhance it" is meant to apply only to small lists, large lists, or both. It's also not clear to me it's an accessibility point at all, vs. a mere responsible-bandwidth recommendation where more than a few hundred select options would be unreasonably intensive/large to generate server-side (which is not applicable for our scenario).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why it would be a good idea to change the behavior depending on the number of options

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.

It's also not clear to me it's an accessibility point at all, vs. a mere responsible-bandwidth recommendation

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.

Copy link
Member

Choose a reason for hiding this comment

The 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:

  • We're stating that AccessibleAutocomplete is inferior than a select dropdown at rendering small lists of items
  • We're accepting an increased maintenance cost in maintaining two separate implementations, and a higher risk of bugs in mind of the fact that a distinct experience is presented on the basis of the number of users on a site (for example, I will rarely encounter AccessibleAutocomplete because my site has fewer than 50 users)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right.

We're stating that AccessibleAutocomplete is inferior than a select dropdown at rendering small lists of items

AccessibleAutocomplete explicitly recommends this type of progressive enhancement: native elements work very well for smaller numbers of elements.

We're accepting an increased maintenance cost in maintaining two separate implementations

Yes, we are accepting maintaining the handling of both states (the implementations themselves are well maintained elsewhere).

Copy link
Contributor

Choose a reason for hiding this comment

The 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` ) }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this will throw an Translate function arguments must be string literals eslint error when attempting to merge master into this branch

#15877

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point @GWWR - fixed in b830022

tStatusResults={ ( length, contentSelectedOption ) => {
return (
_n( '%d result is available.', '%d results are available.', length ) +
' ' + contentSelectedOption
);
Copy link
Contributor

Choose a reason for hiding this comment

The 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 sprintf() here.

} }
cssNamespace="components-post-author__autocomplete"

/>;
}

return (
<PostAuthorCheck>
<label htmlFor={ selectId }>{ __( 'Author' ) }</label>
{ selector }
</PostAuthorCheck>
);
/* eslint-enable jsx-a11y/no-onchange */
Expand All @@ -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(),
};
} ),
Expand Down
94 changes: 94 additions & 0 deletions packages/editor/src/components/post-author/style.scss
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;
Copy link
Contributor

Choose a reason for hiding this comment

The 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, box-sizing is redundant here as it's already being set to border-box for all elements.

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;
}
}
11 changes: 10 additions & 1 deletion packages/editor/src/components/post-author/test/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -41,14 +41,23 @@ describe( 'PostAuthor', () => {
},
};

const postAuthor = [
Copy link
Member

Choose a reason for hiding this comment

The 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.

Copy link
Member Author

Choose a reason for hiding this comment

The 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

Copy link
Member

Choose a reason for hiding this comment

The 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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense - I'll work on documenting what those are first

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense - I'll work on documenting what those are first

What came of this?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm leaning more toward component tests here unless we have a clean example of E2E site setup for 5 users, then 500 on the same site.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The 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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@adamsilverstein happy to add on a few of these, though would you prefer that as a commit to the branch directly, or a child PR?

Thank you for the offer of help :) Please feel free to make commits directly to the branch!

Copy link
Member Author

@adamsilverstein adamsilverstein Jun 23, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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 select can be considered easy to explore from the user's perspective. We can gather feedback and decide later though :)

{
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', {
Expand Down
1 change: 1 addition & 0 deletions packages/editor/src/style.scss
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
@import "./components/page-attributes/style.scss";
@import "./components/panel-color-settings/style.scss";
@import "./components/plain-text/style.scss";
@import "./components/post-author/style.scss";
@import "./components/post-excerpt/style.scss";
@import "./components/post-featured-image/style.scss";
@import "./components/post-format/style.scss";
Expand Down