Skip to content

Hierarchical terms sorted by name and filterable#10138

Merged
notnownikki merged 5 commits intomasterfrom
update/categories-tags-list-ui
Sep 27, 2018
Merged

Hierarchical terms sorted by name and filterable#10138
notnownikki merged 5 commits intomasterfrom
update/categories-tags-list-ui

Conversation

@notnownikki
Copy link
Copy Markdown
Member

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
Copy link
Copy Markdown
Member Author

Looking for accessibility feedback here especially!

const inputId = `editor-post-taxonomies__hierarchical-terms-input-${ instanceId }`;
const filterInputId = `editor-post-taxonomies__hierarchical-terms-filter-${ instanceId }`;

/* eslint-disable jsx-a11y/no-onchange */
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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
) )
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

@afercia
Copy link
Copy Markdown
Contributor

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
Copy Markdown
Member Author

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

@notnownikki
Copy link
Copy Markdown
Member Author

@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
Copy Markdown
Contributor

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
Copy Markdown
Member Author

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

@afercia
Copy link
Copy Markdown
Contributor

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
Copy Markdown
Member

@karmatosed karmatosed left a comment

Choose a reason for hiding this comment

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

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
Copy Markdown
Member Author

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

@notnownikki notnownikki merged commit 8af1f62 into master Sep 27, 2018
@notnownikki notnownikki deleted the update/categories-tags-list-ui branch September 27, 2018 19:19
@karmatosed
Copy link
Copy Markdown
Member

Thanks for all your work!

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

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
Copy Markdown
Contributor

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

@notnownikki
Copy link
Copy Markdown
Member Author

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

@aduth aduth mentioned this pull request Apr 3, 2019
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants