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

Open
wants to merge 51 commits into
base: master
from

Conversation

Projects
None yet
7 participants
@adamsilverstein
Contributor

adamsilverstein commented Jun 19, 2018

Description

Add accessible search with debounced api requests for the author dropdown. Based on https://github.com/alphagov/accessible-autocomplete. accessible-autocomplete is an MIT licensed JavaScript autocomplete from the UK Digital Service built from the ground up to be accessible.

Fixes #7384

Supersedes #5921

How has this been tested?

  • Tested with a dozen users, by adding them with wp-cli: wp user generate --role=editor --count=12
  • Tested with 10100 users by adding them with wp-cli: wp user generate --role=editor --count=10100

Before this change, Gutenberg crashes on my 1 year old macbook pro:

Screenshots

After this change, Gutenberg loads fine and i can quickly search for the correct user. The default CSS included in the component needs some attention, but otherwise this is very usable.

Types of changes

  • Only load the first 100 users from the API at load.
  • Use a debounced search to query the API for matching users.
  • Start searching once the user types 2 characters (the components minLength attribute did not seem to work, so I added a manual test in the search callback).
  • Use progressive enhancement to add accessible-autocomplete when there are more than 50 users to choose from.
  • Remove the filter that enabled sending a -1 to the REST API, which only returned 10000 users in any case. Besides crashing Gutenberg, any users beyond 10000 would not be available. Left this in place for now to avoid breaking other uses (categories, block picker)
  • Include default CSS styles from the accessible-autocomplete project as a starting point - these need work: noted dropdown arrow not showing correctly, font sizes don't match, etc..
  • Add internationalization for all aria strings used by the autocompleter.

Rationale

Although this is a somewhat large PR, I believe we will find many benefits to having an accessible autocomplete component built into. this component is flexible enough to replace any select element we want to turn into a searchable autocomplete. In addition, I believe the use a -1 parameter to perform an unbound query (well, not really, it returned 10000 users) is poor a poor user experience, is bad for performance, and is not scalable.

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.

@adamsilverstein adamsilverstein referenced this pull request Jun 19, 2018

Closed

Post author dropdown: enable search #5921

3 of 3 tasks complete
@adamsilverstein

This comment has been minimized.

Show comment
Hide comment
@adamsilverstein

adamsilverstein Jun 19, 2018

Contributor

@afercia I would appreciate an accessibility review when you have a chance!

Contributor

adamsilverstein commented Jun 19, 2018

@afercia I would appreciate an accessibility review when you have a chance!

@afercia

This comment has been minimized.

Show comment
Hide comment
@afercia

afercia Jul 3, 2018

Contributor

@adamsilverstein seems I'm not able to make this branch work. Maybe a few things have changed in Gutenberg, not sure.

I'd be happy to use the autocompleter from the UK Gov Digital Service, I'm just not able to test this PR 🙂Testing their demo, it works pretty well.

Generally, I'd highly recommend anything that comes from GDS as they're really committed to build accessible solutions. They've also already helped us on #5468 providing some precious feedback. /Cc @aduggin @tvararu

If this solves the performance issue, I'd say go with it!

Contributor

afercia commented Jul 3, 2018

@adamsilverstein seems I'm not able to make this branch work. Maybe a few things have changed in Gutenberg, not sure.

I'd be happy to use the autocompleter from the UK Gov Digital Service, I'm just not able to test this PR 🙂Testing their demo, it works pretty well.

Generally, I'd highly recommend anything that comes from GDS as they're really committed to build accessible solutions. They've also already helped us on #5468 providing some precious feedback. /Cc @aduggin @tvararu

If this solves the performance issue, I'd say go with it!

@adamsilverstein

This comment has been minimized.

Show comment
Hide comment
@adamsilverstein

adamsilverstein Jul 3, 2018

Contributor

seems I'm not able to make this branch work.

Let me refresh it so you can give it a try.

Contributor

adamsilverstein commented Jul 3, 2018

seems I'm not able to make this branch work.

Let me refresh it so you can give it a try.

@tvararu

This comment has been minimized.

Show comment
Hide comment
@tvararu

tvararu Jul 3, 2018

I can see there's two functionality changes being delivered by this work; one is debouncing the search and changing the amount of results being pulled in, and the second is switching to accessible-autocomplete.

These changes touch the same feature but are otherwise independent, so would it make sense to deliver each as its own separate PR? That way the performance improvement can be shipped irrespective of the accessibility enhancements or vice-versa.

(Sorry for parachuting in with a review, feel free to ignore!)

tvararu commented Jul 3, 2018

I can see there's two functionality changes being delivered by this work; one is debouncing the search and changing the amount of results being pulled in, and the second is switching to accessible-autocomplete.

These changes touch the same feature but are otherwise independent, so would it make sense to deliver each as its own separate PR? That way the performance improvement can be shipped irrespective of the accessibility enhancements or vice-versa.

(Sorry for parachuting in with a review, feel free to ignore!)

@afercia

Please see comment.

@afercia

This comment has been minimized.

Show comment
Hide comment
@afercia

afercia Jul 3, 2018

Contributor

Opened an issue on the GDS repo 🙂alphagov/accessible-autocomplete#277

Contributor

afercia commented Jul 3, 2018

Opened an issue on the GDS repo 🙂alphagov/accessible-autocomplete#277

@afercia

This comment has been minimized.

Show comment
Hide comment
@afercia

afercia Jul 3, 2018

Contributor

Parachutes are always appreciated here 😉

Contributor

afercia commented Jul 3, 2018

Parachutes are always appreciated here 😉

@adamsilverstein

This comment has been minimized.

Show comment
Hide comment
@adamsilverstein

adamsilverstein Jul 3, 2018

Contributor

I can see there's two functionality changes being delivered by this work; one is debouncing the search and changing the amount of results being pulled in, and the second is switching to accessible-autocomplete.

is debouncing possible against the current implementation? it is a standard html select element.

Contributor

adamsilverstein commented Jul 3, 2018

I can see there's two functionality changes being delivered by this work; one is debouncing the search and changing the amount of results being pulled in, and the second is switching to accessible-autocomplete.

is debouncing possible against the current implementation? it is a standard html select element.

@karmatosed

This comment has been minimized.

Show comment
Hide comment
@karmatosed

karmatosed Jul 13, 2018

Member

I am checking, does this still need a design?

Member

karmatosed commented Jul 13, 2018

I am checking, does this still need a design?

@adamsilverstein

This comment has been minimized.

Show comment
Hide comment
@adamsilverstein

adamsilverstein Jul 27, 2018

Contributor

I am checking, does this still need a design?

@karmatosed if we go with this solution, we still need to polish the display of the dropdown and give it more polish.

Contributor

adamsilverstein commented Jul 27, 2018

I am checking, does this still need a design?

@karmatosed if we go with this solution, we still need to polish the display of the dropdown and give it more polish.

@andreilupu

This comment has been minimized.

Show comment
Hide comment
@andreilupu

andreilupu Jul 31, 2018

Contributor

I tried to test this since it's something that I would love to see implemented.But unfortunately, I can't make it work because there are some packages moved/removed in the latest Gutenberg core.
And even if I replace api-request with api-fetch (which was deprecated) and if I manually fix other warnings, I get stuck in some block-serialization-spec-parser errors while running npm run dev

Also, wondering if it's OK to recreate this branch from 3.4.0 so I can come back with some real feedback.

Contributor

andreilupu commented Jul 31, 2018

I tried to test this since it's something that I would love to see implemented.But unfortunately, I can't make it work because there are some packages moved/removed in the latest Gutenberg core.
And even if I replace api-request with api-fetch (which was deprecated) and if I manually fix other warnings, I get stuck in some block-serialization-spec-parser errors while running npm run dev

Also, wondering if it's OK to recreate this branch from 3.4.0 so I can come back with some real feedback.

@adamsilverstein

This comment has been minimized.

Show comment
Hide comment
@adamsilverstein

adamsilverstein Sep 20, 2018

Contributor

@andreilupu I'll try to refresh against master so you can test.

Contributor

adamsilverstein commented Sep 20, 2018

@andreilupu I'll try to refresh against master so you can test.

@youknowriad

This comment has been minimized.

Show comment
Hide comment
@youknowriad

youknowriad Oct 10, 2018

Contributor

Hey there, we recently landed this #10138 by @notnownikki which adds a filter input (which was good in terms of accessibility). I wonder if we can combine work and make this same component more reusable and use it for authors as well?

Contributor

youknowriad commented Oct 10, 2018

Hey there, we recently landed this #10138 by @notnownikki which adds a filter input (which was good in terms of accessibility). I wonder if we can combine work and make this same component more reusable and use it for authors as well?

@notnownikki

This comment has been minimized.

Show comment
Hide comment
@notnownikki

notnownikki Oct 15, 2018

Member

Hi all :)

#10138 would need a lot of modification to achieve the goals set out for this PR, as it was centered around selecting multiple terms on an existing checkbox based UI. After looking through the discussions here, I think that getting this working with current master would be the best way forward. Otherwise, we'd just end up re-implementing this PR.

If no one has any objections, I can pick this up and see if I can get it working with current master. Thank you for your work here @adamsilverstein!

Member

notnownikki commented Oct 15, 2018

Hi all :)

#10138 would need a lot of modification to achieve the goals set out for this PR, as it was centered around selecting multiple terms on an existing checkbox based UI. After looking through the discussions here, I think that getting this working with current master would be the best way forward. Otherwise, we'd just end up re-implementing this PR.

If no one has any objections, I can pick this up and see if I can get it working with current master. Thank you for your work here @adamsilverstein!

@adamsilverstein

This comment has been minimized.

Show comment
Hide comment
@adamsilverstein

adamsilverstein Oct 15, 2018

Contributor

@notnownikki that sounds great, thank you! let me know if i can do anything to help get this going again!

Contributor

adamsilverstein commented Oct 15, 2018

@notnownikki that sounds great, thank you! let me know if i can do anything to help get this going again!

@notnownikki

This comment has been minimized.

Show comment
Hide comment
@notnownikki

notnownikki Oct 15, 2018

Member

@adamsilverstein this should all be synced with master now!

I fixed up a bug where changing the select for sites with less than 50 authors didn't update the post's author, so if that was on your list to look at, it's done :)

Member

notnownikki commented Oct 15, 2018

@adamsilverstein this should all be synced with master now!

I fixed up a bug where changing the select for sites with less than 50 authors didn't update the post's author, so if that was on your list to look at, it's done :)

@adamsilverstein

This comment has been minimized.

Show comment
Hide comment
@adamsilverstein

adamsilverstein Oct 16, 2018

Contributor

awesome, thanks @notnownikki! Thanks for helping on this. I'm going to see if I can tell why the travis build is failing and give the latest code a test.

Contributor

adamsilverstein commented Oct 16, 2018

awesome, thanks @notnownikki! Thanks for helping on this. I'm going to see if I can tell why the travis build is failing and give the latest code a test.

@adamsilverstein

This comment has been minimized.

Show comment
Hide comment
@adamsilverstein

adamsilverstein Oct 18, 2018

Contributor
  • Added a little styling to make the dropdown more closely match the rest of Gutenberg, could use some additional polish.
  • re-tested with 10k+ users.
  • fixed broken test by supplying the missing postAuthor data to the test.
Contributor

adamsilverstein commented Oct 18, 2018

  • Added a little styling to make the dropdown more closely match the rest of Gutenberg, could use some additional polish.
  • re-tested with 10k+ users.
  • fixed broken test by supplying the missing postAuthor data to the test.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment