Unify Chip styles #2765
Unify Chip styles #2765
Conversation
Aljullu
added
type: enhancement
Aljullu
force-pushed
the
fix/1786-unify-chip-styles
branch
from
0377ec9
to
ce181b6
Compare
|
Size Change: +5.85 kB (0%) Total Size: 1.59 MB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
Looking much better Albert! I've left some comments inline. Also, while testing, I noticed some slight padding/margin spacing issues (my testing was against Storefront 2.5.8.rc.1):
First, the dismiss "x" for the coupon chips seems to be a bit off center vertically.
Next, the space between the chip and the bottom of the input field (with the filter block) is more than the margin at the top (and the chip doesn't look centered in it's container):
| @@ -25,46 +25,79 @@ const Chip = ( { | |||
| onRemove = () => {}, | |||
| disabled = false, | |||
| radius = 'small', | |||
| removeOnClick = false, | |||
There was a problem hiding this comment.
Took me a few secs to understand the purpose of this prop. At first I thought it was used to single out whether the chip is removable or not, but then I discovered that it's used simply to indicate whether the remove action is attached to the entire chip, or just the "x" element.
So with that in mind, the prop name seems like it could use some "tightening up" so it's purpose is a bit more intuitive. Maybe something like removeOnAnyClick?
Also, I wonder, will there ever be any use-case for using this same Chip Pattern for something that is not removable (meaning the chip is persistently displayed)? Should we differentiate the name of this particular component so it's clear it's always removable (i.e. RemovableChip or DismissableChip)? Eventually the ui/ux could be moved into <Chip> and this component () would wrap around the former enhancing it with the removal actions/labels.
There was a problem hiding this comment.
So with that in mind, the prop name seems like it could use some "tightening up" so it's purpose is a bit more intuitive. Maybe something like
removeOnAnyClick?
👍 Done in ce51de2.
Also, I wonder, will there ever be any use-case for using this same Chip Pattern for something that is not removable (meaning the chip is persistently displayed)? Should we differentiate the name of this particular component so it's clear it's always removable (i.e.
RemovableChiporDismissableChip)? Eventually the ui/ux could be moved into<Chip>and this component () would wrap around the former enhancing it with the removal actions/labels.
Yes, that sounds good to me. Just thinking we should wait until we see the real need to have an un-removable chip before differentiating between two kinds of chips?
There was a problem hiding this comment.
Just thinking we should wait until we see the real need to have an un-removable chip before differentiating between two kinds of chips?
I'd be okay with that, but I just wonder if it would be better to be more explicit now with the name of the component and call it RemovableChip or DismissibleChip which leaves room for having just Chip down the road. It makes it a bit clearer that this is a component with dismissing behaviour?
There was a problem hiding this comment.
I ended up splitting the component in two (Chip and RemovableChip) as you suggested in the first place. This way both components can share the same CSS classes so it's not a breaking change: d0eeb26.
| : sprintf( | ||
| /* translators: %s text of the chip to remove. */ | ||
| __( 'Remove "%s"', 'woo-gutenberg-products-block' ), | ||
| text | ||
| ), |
There was a problem hiding this comment.
Should this be moved as the default for the ariaLabel for incoming props? Then you just have to check if ariaLabel is a string and if not set it to undefined ? Currently the only time __( 'Remove "%s"') will be used is if ariaLabel is set and it isn't of a type string. Plus the value here is different than the default for the prop.
There was a problem hiding this comment.
Hmm, I'm not sure to understand you here. There are three posibilities:
arilaLabelprop is set: in that case, we useariaLabel.ariaLabelis not set:- If
textis a string, it usesRemove "%s". - If
textis not a string, it usesRemove.
- If
That's because text accepts any node, so it could be a string but also an element.
There was a problem hiding this comment.
arilaLabel prop is set: in that case, we use ariaLabel.
In the code it's always going to be set because the destructured prop value has a default:
ariaLabel = __( 'Remove', 'woo-gutenberg-products-block' ),
So the expression ariaLabel || typeof text !== 'string' will never evaluate the second condition unless ariaLabel is explicitly set to something a non-truthy value and not undefined or not set.
The problem I'm seeing might be more clear if you add some tests covering the conditions this expression uses.
There was a problem hiding this comment.
Aaaah, of course! 🤦 I updated the logic in 273fde8. I also made it so it honors the screen reader text. I think it makes more sense to use that one than the default text.
The problem I'm seeing might be more clear if you add some tests covering the conditions this expression uses.
I think it's now covered with the tests added in d0eeb26.
| test( 'should render nodes as the text', () => { | ||
| const component = TestRenderer.create( | ||
| <Chip text={ <h1>Test</h1> } /> | ||
| ); | ||
|
|
||
| expect( component.toJSON() ).toMatchSnapshot(); | ||
| } ); |
There was a problem hiding this comment.
We should switch to use React Testing Library here. I understand though if you want to leave this for now and revisit in a potential follow-up.
|
Thanks for the review @nerrad!
Good catch! I was struggling to get it correctly aligned. Turns out we were using the multiplication X character, which seems to be a bit misaligned to the top. I replaced it with the cancellation X character in 8edd9df:
At some point we could replace it with an SVG icon, but I think the character looks good enough.
Ah, a specificity issue. 🙂 Fixed in: 392e2b1. |
Ya cancellation character definitely looks better 👍 |
|
I haven't reviewed the code yet, but did some testing. With Chrome: Safari: I wiped |
…elying on complex pseudoselector combinations
Co-authored-by: Darren Ethier <darren@roughsmootheng.in>
Co-authored-by: Darren Ethier <darren@roughsmootheng.in>
Aljullu
force-pushed
the
fix/1786-unify-chip-styles
branch
from
d0eeb26
to
2ded0b8
Compare
That's weird, it's working fine for me. To me, it looks like an issue with the font? In 52ca69a I replaced the character with an |
Aljullu
force-pushed
the
fix/1786-unify-chip-styles
branch
from
2ded0b8
to
52ca69a
Compare
| > | ||
| 🗙 |
There was a problem hiding this comment.
Interesting (just noting) it looks like the previous snapshot didn't print the expected char string either. Another indicator it was a good call to switch to an icon
| 🗙 | ||
| <svg | ||
| aria-hidden="true" | ||
| className="dashicon dashicons-arrow-down-alt2" |
There was a problem hiding this comment.
🤔 hmm, this is a bit surprising, shouldn't this be the no-alt icon in the snapshot? Actually in looking closer, it looks like we have bad classnames in the no-alt.js file in our icon library folder. It could be handled in a separate issue (See #2846).
There was a problem hiding this comment.
Ah, good catch! I missed that. Will open a PR today fixing it.
| // 17px is the minimum input field line-height and 14px is the font-size of | ||
| // the drop down selector elements. |
There was a problem hiding this comment.
| // 17px is the minimum input field line-height and 14px is the font-size of | |
| // the drop down selector elements. | |
| // 18px is the minimum input field line-height and 14px is the font-size of | |
| // the drop down selector elements. |
Fixes #1786.
This PR updates the Filter Products by Attribute and Active Filters blocks so they use the
Chipcomponent introduced in the Cart & Checkout blocks.Chipneeded some changes so it could be used in those scenarios.I also took the chance to fix some theme compatibility issues in those blocks (input backgrounds & Clear All color).
We are making some style changes and switching some components, so there is the possibility of themes breakage but I think it's quite small. IMO there is no need for a dev note. Thoughts?
Accessibility
Screenshots
Screenshots with Bistro theme:
Before:
After:
How to test the changes in this Pull Request:
2.1. Try adding new filters.
2.2. Verify chips have the correct styles.
2.3. Try removing them with the keyboard (backspace or Del).
2.4. Try removing them clicking on the chip name.
3.1 Verify chips have the correct styles.
4.1. Verify there are no regression in the Filter Products by Attribute and the Active Filters blocks with other attribute combinations: verify everything is still working and there are no visual bugs.
4.2. Verify there are no regressions with the Chips in the Cart: try adding a coupon in the Cart or Checkout blocks and verify it still looks correct.
Changelog