-
Notifications
You must be signed in to change notification settings - Fork 144
Pills and tooltips in product step of OBW #4707
Conversation
client/profile-wizard/style.scss
Outdated
@@ -263,7 +263,7 @@ | |||
} | |||
|
|||
label.components-checkbox-control__label { | |||
margin-left: $gap-large - $gap-smallest; | |||
margin-left: $gap; |
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.
This is just a small tweak to get the label to line up with the description per the design mockup
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.
Looking really great Bec. Couple of minor comments in the code, otherwise all looking good there.
I did have a few questions on the design side of this so cc @jameskoster :
- I verified with the design that the pills should be purple. Given that we have been moving away from purple everywhere else, just wanted to see why we are keeping it in this new component?
- On narrow view ports, we can end up with the pill wrapping onto a second line. How should we handle this?
|
||
const monthlyPrice = ( yearlyPrice / 12.0 ).toFixed( 2 ); | ||
const priceDescription = sprintf( | ||
'$%f per month, billed annually', |
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 like we missed a translation wrapper on this text.
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.
Whoops, I thought that since sprintf
came from the i18n library it included translation. Fixed in 6156c3f.
@@ -0,0 +1,7 @@ | |||
export function Pill( { children } ) { |
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.
A new component is born 🎉 - can we also get a line added to the changelog about this new component?
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.
Done in 0fe3e6d
I'm going to confirm this with the design team in our call on Tuesday. |
@becdetat we decided that in this use case we'll make the pills gray :D If you could use |
Thanks @jameskoster, I've updated the screenshot above with the new colours. Do you have any guidance around how to handle narrow viewports and the pill pushing to the next line? |
Ah, sorry! Yes, I think the wrapping is ok, so long as we can fix the justification. When wrapped, if the title, pill, and description all line up nicely I think we're good :) |
@jameskoster can you check the small viewport screenshot in the description above? Everything looks nicely aligned now. |
@jameskoster just a follow-up to see if you can check on the layout? Cheers :-) |
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.
Code is looking great.
Check that the layout matches the design mockup
The continue button has drifted left somehow.
If I remember correctly we support all the way to 320px wide displays. The pills could use just a touch of smallerness.
I also noticed the designs have grey lines and slightly more padding between options:
@@ -0,0 +1,8 @@ | |||
.woocommerce-pill { | |||
border: 1px solid $medium-gray-text; |
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.
FYI medium-gray-text
is getting removed in the next update of base-styles in #4759.
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.
Good catch, let's make that gray-700
if we can :)
return description; | ||
} | ||
|
||
const monthlyPrice = ( yearlyPrice / 12.0 ).toFixed( 2 ); |
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.
TIL toFixed
😄
Dang, that is annoying! I'm gonna say I'm ok with it though as it feels like an edge case to me. If we can just make sure the pills use the Caption text style (including the line-height) that will at least help a little bit. |
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.
Could we only adjust the font-size down to 11px on small screens? That way it will maintain consistency with the caption
text style on all other viewport sizes :)
@jameskoster done in 38ccaa0 with a breakpoint on screens 320px and smaller. |
@jameskoster could you give this one final 👀 from the design side of things please? |
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.
Took for one last spin, all is looking great 🚢
… into pills-and-tooltips-in-obw # Conflicts: # client/profile-wizard/steps/product-types.js
Partially addresses #4577
This replaces the existing price text with a new pill component, using per-month amounts. It also adds a tooltip to each pill.
Accessibility
Screenshots
Smaller viewport:
Detailed test instructions: