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

Hierarchical terms sorted by name and filterable #10138

Merged
merged 5 commits into from Sep 27, 2018

Conversation

@notnownikki
Copy link
Member

@notnownikki notnownikki commented Sep 24, 2018

Description

Fix for #2607

Limits the height of the list so that sites with a lot of categories don't have lists that cause the page to grow a lot.

Adds a filter field to do simple substring matching on the list.

Changes the default ordering to name ascending.

How has this been tested?

Load a post, check the categories are there. Try filtering the list of categories to find the one you want.

Screenshots

newcategoryui

Types of changes

New feature (non-breaking change which adds functionality)

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
@notnownikki notnownikki requested review from karmatosed and afercia Sep 24, 2018
@notnownikki
Copy link
Member Author

@notnownikki notnownikki commented Sep 24, 2018

Looking for accessibility feedback here especially!

@@ -268,10 +282,24 @@ class HierarchicalTermSelector extends Component {
const noParentOption = `— ${ parentSelectLabel } —`;
const newTermSubmitLabel = newTermButtonLabel;
const inputId = `editor-post-taxonomies__hierarchical-terms-input-${ instanceId }`;
const filterInputId = `editor-post-taxonomies__hierarchical-terms-filter-${ instanceId }`;

/* eslint-disable jsx-a11y/no-onchange */

This comment has been minimized.

@afercia

afercia Sep 24, 2018
Contributor

this can be removed as jsx-a11y/no-onchange applies only to <select> elements


/* eslint-disable jsx-a11y/no-onchange */
return [
...this.renderTerms( availableTermsTree ),
<input

This comment has been minimized.

@afercia

afercia Sep 24, 2018
Contributor

the search input needs a visible <label> element, properly associated with for / id attributes

@@ -1,3 +1,8 @@
.editor-post-taxonomies__hierarchical-terms-list {
max-height: 14em;
overflow: auto;

This comment has been minimized.

@afercia

afercia Sep 24, 2018
Contributor

scrollable divs need to be made keyboard accessible: only Firefox makes them focusable by default so for other browsers it's necessary to make them focusable and add some ARIA. See for example the Inserter. I'd say something like tabIndex="0" role="group" aria-label={ __( 'Available terms' ) }" could work

( childTerm ) => -1 !== childTerm.name.toLowerCase().indexOf( filterValue.toLowerCase() )
).reduce(
( appeared, appears ) => appeared || appears
) )

This comment has been minimized.

@afercia

afercia Sep 24, 2018
Contributor

Seems all this will work only for children. What about grandchildren and deeper nested terms?

@afercia
Copy link
Contributor

@afercia afercia commented Sep 24, 2018

Great to see exploration on this. I've left some comments, focussed on a11y and functionality. Will leave other considerations to the expertise of the GTeam.

In addition to the comments, some more considerations:

  • there's no audible feedback about the results of the filtered search: all the other searches / completers in Gutenberg also use speak to send an audible message to assistive technologies with the number of results or "no results"
  • when there are no results, in addition to the audible message, should some text be used to communicate "no results" also visually?
  • as said in the comments, seems it doesn't work for terms nested deeper than 1 level: I'm using the themes unit test data and I have grandchild categories:

screen shot 2018-09-24 at 19 19 08

typing "grandchild" doesn't return any result though:

screen shot 2018-09-24 at 19 18 52

  • say I have only 4-5 categories: should the search field be displayed? the categories will be all displayed because they don't exceed the max-height and a filter functionality seems a bit pointless in this case
@notnownikki
Copy link
Member Author

@notnownikki notnownikki commented Sep 24, 2018

@afercia thank you! I think I can take care of all of that :)

@notnownikki
Copy link
Member Author

@notnownikki notnownikki commented Sep 26, 2018

@afercia I think I've addressed the concerns now, although I'm not sure how to test the speech. I get the feeling it might need to be debounced, but I'm not sure.

@afercia
Copy link
Contributor

@afercia afercia commented Sep 26, 2018

@notnownikki thanks! Oh yes it needs to be debounced otherwise a message is sent at any keystroke. I think withSpokenMessages already exposes both a speak and a debouncedSpeak prop. See other usages in the codebase.

@notnownikki
Copy link
Member Author

@notnownikki notnownikki commented Sep 26, 2018

Aha, so it does! Ok, using the debounced version now.

@afercia
Copy link
Contributor

@afercia afercia commented Sep 26, 2018

not sure how to test the speech

  • use a screen reader :)
  • observe the change in the content of the wp-a11y-speak-region div element
  • there used to be a plugin to log in the console the speak messages, built by @westonruter not sure if it works with the new version of speak
Copy link
Member

@karmatosed karmatosed left a comment

Design wise to me this works as a start to fix this problem. I am torn on suggesting more padding top/bottom but we can iterate. Thanks for this improvement.

@notnownikki
Copy link
Member Author

@notnownikki notnownikki commented Sep 27, 2018

Awesome! @karmatosed thank you so much for your guidance on this ❤️

@notnownikki notnownikki merged commit 8af1f62 into master Sep 27, 2018
2 checks passed
2 checks passed
codecov/project 48.63% (-0.18%) compared to fd9bfa5
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@notnownikki notnownikki deleted the update/categories-tags-list-ui branch Sep 27, 2018
@karmatosed
Copy link
Member

@karmatosed karmatosed commented Oct 2, 2018

Thanks for all your work!

@mtias mtias added this to the 4.0 milestone Oct 9, 2018
@mtias
Copy link
Contributor

@mtias mtias commented Oct 9, 2018

This is great, thanks for the improvement. What do you think about not showing the search box if there are less than 8 items or so?

cc @jasmussen @karmatosed @melchoyce @kjellr @alexislloyd

@jasmussen
Copy link
Contributor

@jasmussen jasmussen commented Oct 9, 2018

I think that seems good. Want to make sure removing the search box doesn't mess with the tabbing.

@notnownikki
Copy link
Member Author

@notnownikki notnownikki commented Oct 9, 2018

PR that does that is up at #10438 for you to try :)

@youknowriad youknowriad mentioned this pull request Oct 10, 2018
4 of 4 tasks complete
@aduth aduth mentioned this pull request Apr 3, 2019
7 of 7 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.