-
Notifications
You must be signed in to change notification settings - Fork 218
ProductAttributeControl: Polish style, screen reader interaction #412
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Pre-approving pending one design question.
&.is-loading { | ||
justify-content: center; | ||
|
||
.components-spinner { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be horizontally centered?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couple of small issues;
- The count on the attribute rows should have a different treatment to the count on the term rows as they're counting different things. See here for the latest design. If adding the label to the count on the attribute row is problematic (or a lot of work) then we should just remove that count - the product count on the term row is more important.
- Speaking of which, I'm not seeing the product count on the term rows.
- I'm not seeing the up/down chevrons. Latest Chrome. https://cloudup.com/c4KqFiX_CSH
I was basing it off this design linked in #405 - I assumed that one was the latest. Now I see that that's the Featured Product design 🙃 Does that mean the background for the terms should be that darker color, with borders between?
I can easily add the product count back to the terms. It's not built to allow for different styling of the count though - but that would also mean we can't do it on the Featured Product variations count, so if that's important we can look at changing the SearchListControl (but not for this iteration).
Strange– if you inspect element in the empty space next to the count, do you see the |
No, the current row styling is good :)
Let's just go ahead and remove the terms count. I'm not sure how useful that info is anyway. We can revisit when we add variation support to the featured product block.
Yeah the code looks good. For some reason the icon just isn't rendering. I only have one browser extension which I disabled - no dice. It works in Safari. Other instances of svg's as background (e.g. https://codepen.io/netsi1964/pen/HGJms) are working fine. Weird. |
…API requests for previously fetched terms. Add spinner to loading items.
2e2029c
to
475f8cd
Compare
updated the row counts for attributes & terms
I'm going to go ahead and merge this since the design feedback has been addressed. We can make any "last-minute" tweaks on Monday before releasing 1.4 if we need to. |
Following up from #405 - cleans up the styling to match the design & fix missing aria info for screen reader users.
Accessibility
Screenshots
Example of an attribute with 2 terms selected
On the initial load, all attributes are collapsed. If an item has no terms, it's disabled (unclickable and dimmed)
EDIT: forgot to add a "Before" - this is what it looks like on master now:
How to test the changes in this Pull Request: