Add dropdown display style to Attribute Filter block #1255

Aljullu · 2019-11-25T19:08:40Z

This PR implements the dropdown display style for the Filter Products by Attribute block with the Query Type set to OR (see #1134 for the difference between OR and AND).

I plan to implement the AND format in a follow-up, but wanted to get feedback early before starting with it.

Accessibility

I've tested using only a keyboard (no mouse)
I've tested using a screen reader
All text has at least a 4.5 color contrast with its background

Screenshots

How to test the changes in this Pull Request:

Create a post with a Filter Products by Attribute block and select the Dropdown option in Display Style.
Preview the post and interact with the filter (search terms, add them, remove them, repeat only using the keyboard, using a screen reader, etc.).
Verify everything works as expected.

Follow-ups

As mentioned, the AND dropdown format will be implemented in a follow-up.
I didn't implement the Apply button that can be seen in the mockups. I would like to discuss with @jwold not showing it or making it optional as in the Price Filter, so instant updates can happen.

Changelog

Add dropdown display style to Filter Products by Attribute block.

nerrad · 2019-11-28T16:14:57Z

I'm getting validation errors on existing usage of Filter by Attribute block. Since this block has been released, I think we need to ensure there are no validation errors.

Aljullu · 2019-11-28T16:40:05Z

You're right @nerrad! That should be fixed in the last commit (fae6b71).

nerrad

Great job and tests well 👍 I have a few comments and a couple accessibility issues. There were a number of other accessibility issues showing up using the Axe Tool Extension but I didn't list them all here. Some are related to storefront or outside the scope of this pull. Would you mind fixing the ones applying to this specific pull and maybe document others that show up (I'm even fine with creating an issue to use the tool to audit things for what needs fixed).

Accessibility issues

The selected item chips do not have a high enough color contrast ratio for accessibility:

When I use the keyboard to remove a selected item (chip) by tabbing over the chip and hitting enter, the focus is lost from the input (and I have to hit tab again to return focus to the input). However backspacing to remove a chip works as expected.

This DropDown selector component is something that might be a good candidate for wider distribution. I wonder if we should start keeping a list of those somewhere?

nerrad · 2019-11-29T11:16:45Z

assets/js/base/components/checkbox-list/index.js

 			<Fragment>
 				{ options.map( ( option, index ) => (
-					<Fragment key={ option.key }>
+					<Fragment key={ option.value }>


I'm curious why this change? I know you changed the expected shape from option.key to option.value (so option.key no longer exists`) Is this just to make it more consistent (being it is the value for the option)?

There is no reason in fact, I modified it initially because the Downshift example was using value instead of key and to get started it was easier to modify the data structure of options.

Later, I didn't undo the change because both key and value sound good to me. But I don't mind undoing it if you think key was better.

nerrad · 2019-11-29T11:24:34Z

assets/js/base/components/dropdown-selector/index.js

+	onChange = () => {},
+	options = [],
+} ) => {
+	const inputRef = useRef( null );


The same inputRef value is implemented on multiple inputs. Was that intentional?

Good catch! That was probably a copy-&-paste error because <DropdownSelectorInputWrapper> was not using the inputRef prop. Removed it in 7f77564.

nerrad · 2019-11-29T11:40:23Z

Oh I also forgot to add that when I run npm install in this branch, I get a package-lock.json diff (which includes the downshift install). Are you pushing package-lock.json changes?

Aljullu · 2019-11-29T14:12:18Z

Thanks for the review @nerrad. I fixed all issues I found in AXE related to this PR and opened an issue to audit the other blocks: #1281.

The selected item chips do not have a high enough color contrast ratio for accessibility:

Fixed in 5a6b7eb.

When I use the keyboard to remove a selected item (chip) by tabbing over the chip and hitting enter, the focus is lost from the input (and I have to hit tab again to return focus to the input). However backspacing to remove a chip works as expected.

Fixed in 85c3cfd.

Oh I also forgot to add that when I run npm install in this branch, I get a package-lock.json diff (which includes the downshift install). Are you pushing package-lock.json changes?

I was sure I added it but it looks like I didn't. 🤦‍♂️ Done in f313d65.

nerrad

Color contrast is fixed.

However I still see this:

Also, there's still a loss of focus when tabbing and hitting enter to remove a selected item - here's a gif that might help (but it doesn't show the key presses...highlighted chip is when the tab is focused on it).

Aljullu · 2019-11-29T18:11:08Z

However I still see this:

Good catch! I missed the case when menu was closed. Should be fixed in f5b7e57.

Also, there's still a loss of focus when tabbing and hitting enter to remove a selected item - here's a gif that might help (but it doesn't show the key presses...highlighted chip is when the tab is focused on it).

The GIF was helpful, thank you! I think I misunderstood you in the previous message. This issue should be fixed in de78883.

nerrad

🎈 🎉 🌮 ....

…ackspace

…ly disabled

…ip card

jwold · 2019-12-02T19:49:37Z

@Aljullu I think having the apply button as optional (but turned off), makes a lot of sense. Great work!

Aljullu self-assigned this Nov 25, 2019

Aljullu added the status: in progress label Nov 25, 2019

Aljullu force-pushed the add/attributefilter-dropdown-display branch 3 times, most recently from a0024ac to 63f189b Compare November 27, 2019 14:48

Aljullu added status: needs review type: enhancement The issue is a request for an enhancement. and removed status: in progress labels Nov 27, 2019

Aljullu requested a review from a team November 27, 2019 15:04

Aljullu mentioned this pull request Nov 27, 2019

Multiselect checkbox/tag input component #1134

Closed

Aljullu marked this pull request as ready for review November 27, 2019 16:04

nerrad suggested changes Nov 29, 2019

View reviewed changes

Aljullu force-pushed the add/attributefilter-dropdown-display branch 4 times, most recently from e618124 to da9ac3b Compare November 29, 2019 14:07

Aljullu requested a review from nerrad November 29, 2019 14:12

nerrad reviewed Nov 29, 2019

View reviewed changes

Aljullu force-pushed the add/attributefilter-dropdown-display branch from efb38f9 to f5b7e57 Compare November 29, 2019 18:08

nerrad approved these changes Nov 29, 2019

View reviewed changes

woocommercebot added status: ready to merge and removed status: needs review labels Nov 29, 2019

Aljullu added 3 commits December 2, 2019 11:28

Add dropdown display style to Attribute Filter block

1e78eaf

Unify filter blocks margin

b96f4b3

Show attribute label inside dropdown input

310daaa

Aljullu and others added 12 commits December 2, 2019 11:28

Minor CSS reorganization

c7f5740

Refactor code to smaller files

ce877e3

Preserve input values on blur

369f978

Only save data-display-style if it's different than 'list'

6b77114

Remove inputRef prop in DropdownSelectorInputWrapper

87e9f27

Accessibility: fix missing label

7a68486

Prevent input field being unselected when removing an item with the b…

0e8cdf6

…ackspace

Remove isLoading styles and don't set isDisabled when it's not actual…

94b7d13

…ly disabled

Accessibility: increase color contrast

6be4243

Add package-lock.json

5a4ff2a

Prevent input field being unfocused when removing an item with its ch…

8f1f526

…ip card

Don't show menu when input is unfocused

667838f

Aljullu force-pushed the add/attributefilter-dropdown-display branch from f5b7e57 to 667838f Compare December 2, 2019 10:28

Aljullu mentioned this pull request Dec 3, 2019

Add 'AND' display format to Attribute filter dropdown #1309

Merged

3 tasks

Aljullu merged commit 7ac5b29 into master Dec 3, 2019

Aljullu deleted the add/attributefilter-dropdown-display branch December 3, 2019 14:33

Aljullu mentioned this pull request Dec 5, 2019

Add 'Go' button to Attribute Filter #1332

Merged

nerrad added this to the 2.6.0 milestone May 25, 2020

Add dropdown display style to Attribute Filter block #1255

Add dropdown display style to Attribute Filter block #1255

Uh oh!

Conversation

Aljullu commented Nov 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Accessibility

Screenshots

How to test the changes in this Pull Request:

Follow-ups

Changelog

Uh oh!

nerrad commented Nov 28, 2019

Uh oh!

Aljullu commented Nov 28, 2019

Uh oh!

nerrad left a comment

Choose a reason for hiding this comment

Accessibility issues

Uh oh!

nerrad Nov 29, 2019

Choose a reason for hiding this comment

Uh oh!

Aljullu Nov 29, 2019

Choose a reason for hiding this comment

Uh oh!

nerrad Nov 29, 2019

Choose a reason for hiding this comment

Uh oh!

Aljullu Nov 29, 2019

Choose a reason for hiding this comment

Uh oh!

nerrad commented Nov 29, 2019

Uh oh!

Aljullu commented Nov 29, 2019

Uh oh!

nerrad left a comment

Choose a reason for hiding this comment

Uh oh!

Aljullu commented Nov 29, 2019

Uh oh!

nerrad left a comment

Choose a reason for hiding this comment

Uh oh!

jwold commented Dec 2, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Aljullu commented Nov 25, 2019 •

edited

Loading