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

Aljullu · 2019-12-03T11:05:41Z

Fixes #1134.
Fixes #1135.

Follow-up of #1255.

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:

Add a Filter Products by Attribute block to a post and set its Display Style to Dropdown and the Query Type to AND.
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.
Verify there are no regression in the same component when Query Type is set to OR.

nerrad · 2019-12-03T13:36:19Z

So to clarify (going by by the gif alone), is AND intended to mean that you can only select one attribute term to query by? Why is this the route taken instead of AND meaning all the selected terms must exist vs OR where any of the terms exist?

It's quite possible this is a limitation of attribute term queries for Woo that I'm not aware of

Aljullu · 2019-12-03T14:07:53Z

So to clarify (going by by the gif alone), is AND intended to mean that you can only select one attribute term to query by? Why is this the route taken instead of AND meaning all the selected terms must exist vs OR where any of the terms exist?

It's quite possible this is a limitation of attribute term queries for Woo that I'm not aware of

@nerrad that's to keep feature-parity with the current widgets. But I would love to do research at some point and see if this can be improved so instead of tying up the dropdown style to the Query Type, we add another attribute to allow single/multiple select. So you could have an AND dropdown that allows selecting multiple filters.

Is that something we might want to pursue in the short-term? I would create an issue if that's the case.

For reference: p6riRB-4CG-p2.

nerrad · 2019-12-03T15:09:58Z

Is that something we might want to pursue in the short-term? I would create an issue if that's the case.

I think we should consider it. While I appreciate at a minimum reaching feature parity with existing widgets, I also think we have an opportunity to introduce new enhancements that make the blocks more appealing over existing widgets (which can potentially help with adoption).

So for clarity, I see the following query options:

OR which allows for selecting multiple items to query by (list and dropdown behave similarly). This means that joins are including results that match against any of the terms.
AND with single selection: allows for selecting a single terms to query by (list becomes radios, dropdown is single select). This means that joins are including a match on that selected term which must be present.
AND with multiple selection: allows for selecting multiple terms to query by (list becomes checkboxes, dropdown is multiple select). This means that joins are including a match against ALL selected terms which must be present in the results.

I don't know if such queries are even feasible with the Woo data model, so the issue should explore that as well.

All of the above can be copied into the issue.

This particular pull then is concerned with item 2 (I'll review from that context).

Aljullu · 2019-12-03T15:36:50Z

AND with single selection:

Actually, if we only allow single selection, it doesn't matter if it's AND or OR, right? So I would hide the Query Type option in that case. Something like this:

I also wonder if we should allow the List display style with single selection. We could implement it with radio boxes. Is that something that would be useful?

nerrad · 2019-12-03T15:41:46Z

it doesn't matter if it's AND or OR, right? So I would hide the Query Type option in that case.

I think it does. OR is always multiple selection. However "AND" could be either single or multiple depending on how the selections apply to the final query eg. And: "Blue AND Yellow" Or: "Blue OR Yellow". So one way of doing it would be to flip the controls around so that if they select AND, there is a Single or Multiple controls. If it's "OR" then no single/multiple control.

Should we move this specific convo to a new issue though or were you thinking of just getting it done in this pull (which I'd be fine with assuming it won't take too much more time and the resulting query is okay to do server side).

nerrad

Just a few comments, but otherwise looking good. I've reviewed this assuming we would merge as is and continue the conversation about multiple selections for an AND query type in a separate issue. Obviously things will change if you decide to implement that in this pull.

nerrad · 2019-12-03T15:17:10Z

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

+	const stateReducer = ( state, changes ) => {
+		switch ( changes.type ) {
+			case Downshift.stateChangeTypes.keyDownEnter:
+			case Downshift.stateChangeTypes.clickItem:
+				return {
+					...changes,
+					highlightedIndex: state.highlightedIndex,
+					isOpen: multiple,
+					inputValue: '',
+				};
+			case Downshift.stateChangeTypes.blurInput:
+			case Downshift.stateChangeTypes.mouseUp:
+				return {
+					...changes,
+					inputValue: state.inputValue,
+				};
+			default:
+				return changes;


One of those times where I wish we had performance measuring tools to know whether we should wrap this in useCallback (memoized on the multiple prop). I'm on the fence. Being this is a reducer passed through to Downshift, and in particular the wrapping component, I think there's value in wrapping this in useCallback here. Especially since parent components in a tree using this component are likely to re-render (filters due to attribute count changes?)

Converting it to useCallback makes sense, I did the change in 9c7c8c0.

nerrad · 2019-12-03T15:37:58Z