Filter all products block by attribute

[mikejolley]

Introduces an attribute filter block for use alongside the all products block.

Ref: #713 Note some acceptance criteria in that issue are not met in this MVP.

TODO:

• Convert to store namespace
• The list should be truncated if it contains more than 10 attributes
• Add attribute selector to inspector sidebar as per the mocks
• Discuss headings and 'toggle' in acceptance criteria

Screenshots

How to test the changes in this Pull Request:

The best way to test these in conjunction with other filters is as follows:

1. Create a column block with 2 columns
2. Add the all products block to one column
3. Add a price slider, and 2 attribute filter blocks to the other column. Setup one to use attribute 'size' and one to use 'color'. This is assuming you are using woo core sample data. You can use heading blocks to label them if you wish.

Then you can test how the filters work on the frontend.

Changelog

Introduce a filter products by attribute block which works alongside the new "All products" block.

[Aljullu]

This is looking amazing, you did huge progress these last days, congrats! 👏

The behavior is still a bit different from core. Using AND as the Query Type, I can reach a point that the block completely disappears:

[nerrad]

Nice work! Have a few comments sprinkled throughout for your perusal 👇

[Aljullu]

Thanks for applying the feedback @mikejolley! Some minor things I found:

[nerrad]

Almost there! It'd be nice to break up monolith pulls like this if possible, the REST API endpoint could have gone in it's own pull (It should have tests too). However, I reviewed the REST API here so no need to split out now. Comments below 👇

[nerrad]

Fragment shouldn't be needed here, just put the key on the <li>

[mikejolley]

This fragment wraps 2 sibling <li> elements so is needed here?

[nerrad]

You're right there is one Fragment that could remain. However there's a couple things:

1. The inner level fragment should still not be needed.
2. The key should be on the li items, not the Fragment. The problem with the Fragment having the key is that the "show more options" toggle will get re-rendered everytime the inner list re-renders even if it's not necessary.

The above points might not see important in the context of this particular usage, but it's pattern that could be more problematic in other contexts if it becomes a habit :). So I'm just promoting best practices here.

This code could get changed to:

useMemo( () => {
	// Truncate options if > the limit + 5.
	const optionCount = options.length;
	const shouldTruncateOptions = optionCount > limit + 5;
	return (
		<Fragment>
			{ options.map( ( option, index ) => (
				<li
					key={ option.key }
					{ ...shouldTruncateOptions &&
						! showExpanded &&
						index >= limit && { hidden: true } }
				>
					<input
						type="checkbox"
						id={ option.key }
						value={ option.key }
						onChange={ onCheckboxChange }
						checked={ checked.includes( option.key ) }
					/>
					<label htmlFor={ option.key }>{ option.label }</label>
				</li>
			) ) }
			{ shouldTruncateOptions &&
				options.length > limit &&
				renderedShowMore }
			{ shouldTruncateOptions && renderedShowLess }
		</Fragment>
	);
}, [
	options,
	checked,
	showExpanded,
	limit,
	onCheckboxChange,
	renderedShowLess,
	renderedShowMore,
] );

Even better though, would be to handle the renderedShowMore and renderedShowLess outside of this memoized function. I'd combine the renderedShowMore and renderedShowLess into one memoized function that spits out either of those or null based on the conditions. Then you could get rid of the last unnecessary fragment and end up with something like this:

<ul className={ classes }>
    { isLoading ? placeholder : renderedOptions  }
    { renderedToggleOptions }
</ul>

[mikejolley]

Hmm but by having the show more after the list items, if you tab through (with keyboard) when it expands you're at the bottom of the list rather than from the point of expansion.. that was my justification for having it appear inline. Originally I did it similar to you but only rendering visible list items. You were against that particular solution also.

[nerrad]

🤔 hmm... I see your point. I tried both behaviours and what you have definitely works better from an a11y pov. Let's roll with it.

[mikejolley]

The problem with the Fragment having the key is that the "show more options" toggle will get re-rendered everytime the inner list re-renders even if it's not necessary.

Did we work around this by adding a key to the show more/show less li?

[nerrad]

Did we work around this by adding a key to the show more/show less li?

Hmm... I don't think so (not sure honestly), but regardless I think in this case any accessibility improvements make it worth potential extra rendering for the time being (until we can think of a better pattern).

[nerrad]

should you still do a check here and return a default queryState if taxonomy is null? That will guard against any the lack of any taxonomy for the given attribute id coming into this component.

[mikejolley]

Without a taxonomy this query will be broken... I think the change to the useCollection hook would be good so we can avoid the query altogether if missing (since we cannot add a conditional before the hooks). Follow up?

[nerrad]

👍 to follow up.

[nerrad]

Thanks for handling the feedback 👍 I still see a couple things here that need addressed. Also, should there be tests for the new REST endpoints or are you handling that in a separate pull?

[mikejolley]

@nerrad I'll add tests separately if we're happy with the API changes. R/e the useCollection follow-up, are you handling it?

[nerrad]

@nerrad I'll add tests separately if we're happy with the API changes

👍

R/e the useCollection follow-up, are you handling it?

If you mean adding a callback to indicate whether to do the query or not, yup I can. Would you mind doing the issue though? I think you'll be able to describe the problem we're solving by that in the description pretty well. You can just assign to me after?