Product Categories List: Update dropdown view

[ryelle]

This PR updates the Product Categories List block, dropdown view. It adds the hierarchy support back in, and enables navigation using the dropdown.

Accessibility

Note: I've had to add a button to trigger the navigation. The other options are onBlur or onChange on the select element. Both have issues– onBlur works better using a keyboard, you can select a category and the navigation triggers once you leave the element; but for a mouse user you need to click away from the select box and only then will it navigate. onChange works as expected for a mouse user, but on some browsers (IE11 is what I tested) any keyboard nav just scanning through the select options triggers the navigation, so it will always nav to the first item, never letting you get through the options.

Since both of those approaches have downsides, I decided to follow the "best practice" outlined here: use a button to trigger the navigation. A button also makes it clear that this is a navigation action, which might not be as clear with the dropdown alone.

• I've tested using only a keyboard (no mouse)
• I've tested using a screen reader

Screenshots

In the editor, you see the dropdown preview + button, but clicking the button won't navigate.

You can open the dropdown, which helps you see the preview when, for example, hierarchy and product counts are enabled.

On the front end, it looks pretty much the same.

If you select a category, you can click the button now to navigate to that archive.

How to test the changes in this Pull Request:

1. Add Product Categories List block to you post/page
2. Set it to show as a dropdown
3. Change any other settings as you want
4. It should behave as you expect
5. View the front end
6. It should look the same as the editor preview, and selecting a category should navigate you to that page.

[ryelle]

@jwold Looks like I can't request a review from you until you accept the invite to WooCommerce? In any case, this is the PR I mentioned earlier, where I had to deviate from the design for accessibility reasons. Would like your input on the button solution 🙂

[Aljullu]

This is a great improvement for the Product Categories List block. I tested it and everything works fine and code looks good. I added a couple of questions in the code below but pre-approving pending for @jwold's input.

Since both of those approaches have downsides, I decided to follow the "best practice" outlined here: use a button to trigger the navigation. A button also makes it clear that this is a navigation action, which might not be as clear with the dropdown alone.

Agree this is the best solution. 👍

[Aljullu]

Is there any case that 0 === url.indexOf( home ) will evaluate to false? I'm unable to see it and I'm curious now. 🙃

If there is, what do you think about rewriting this check as:

Suggested change

	if ( ! isPreview && 0 === url.indexOf( home ) ) {
	if ( ! isPreview && url.startsWith( home ) ) {

[ryelle]

IE doesn't support string.startsWith. Why do you think we'd need to rewrite it? This check is make sure all links are internal, so url should always start with home.

[Aljullu]

IE doesn't support string.startsWith.

I thought transpiling the code with Babel would take care of IE11 compatibility, am I missing something? When testing it, I see strings get the startsWith method and it works just fine even in IE11:

Why do you think we'd need to rewrite it?

There is no need to change it, indexOf works too. It was just that I usually prefer ES6 methods because I think they make code easier to read for newcomers, but it's just a question of taste. 🙂

This check is make sure all links are internal, so url should always start with home.

Right, I was curious if there is any case in which a category link could be external.

[ryelle]

weird, I was just going on with what MDN said in its support matrix, and made the assumption since it didn't add forEach to NodeList that it wouldn't do this either 🤷🏼‍♀️

if there is any case in which a category link could be external.

I don't think that's possible, since we're linking off to WP-created taxonomy archives. I suppose a plugin could interject and do anything it wanted, but that seems like it would be really edge case.

[mikejolley]

Nice job, this works great.

The only issue I see is the button does nothing (no feedback) when a category is not selected. I can think of 2 solutions for this which avoid modals;

1. Don't show the empty "select a category" value and just have the top most (or current, thinking ahead to when this may be used in a widget area) by default
2. Make the default state of the navigation button disabled with a modified title with instructions to select a cat.

or something else :)?

[ryelle]

The only issue I see is the button does nothing (no feedback) when a category is not selected. I can think of 2 solutions for this which avoid modals;

I don't think this a problem– if you haven't selected a category, there is no page to navigate to, so the button doesn't go anywhere. This seems like expected behavior to me.

[jwold]

@ryelle good catch on the accessibility front. I'm good with moving this forward! Thanks :)

I do have two things I'd like to understand more of (apart from this PR):

1. Do select fields as a rule need a button from an accessibility standpoint? Curious if there's a way around that.
2. Is the caret a specific rule we should follow, or would something like "Go" be ok? More thinking out loud here, I'm not sure which I prefer.

[ryelle]

Do select fields as a rule need a button from an accessibility standpoint? Curious if there's a way around that.

The issue is triggering an action when the select box changes. So a dropdown in the context of a form, like picking a state on a shipping form should be fine. If the form changes based on that selection, it would probably depend on how disruptive it would be to have the form change as someone is scanning through the options. In this case, it triggers a page navigation, so that's as disruptive as you can be - so we move the trigger off to a button.

I know most designers hate to get WCAG citations, but this one falls under "3.2.5 Changes of context are initiated only by user request or a mechanism is available to turn off such changes." – it's a level AAA requirement, so we don't need to match it (best practice is to match level A and level AA). I think we can be a little more discerning about it (judging based on the impact to the user, rather than "always do XYZ").

Is the caret a specific rule we should follow, or would something like "Go" be ok? More thinking out loud here, I'm not sure which I prefer.

Nope, I just wanted something that would be clear– the caret/IconButton pattern lets me have a simple button with a more verbose label than just "Go", which is unclear out of context. Links, inputs, and buttons need to have clear labels since it's possible for a screen reader user to navigate through a list of all the form controls on a page.

[jwold]

I don't mind the citation at all actually! I take it as an opportunity to learn something new.

Thanks for that explanation. I do appreciate the distinction between it being a AAA, but also that makes such a disruptive change that it could be argued it warrants a button to trigger the change more so than something smaller.