-
Notifications
You must be signed in to change notification settings - Fork 69
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Checkout: Make sure the font size for PMME is smaller than the labels #9925
base: develop
Are you sure you want to change the base?
Conversation
Test the buildOption 1. Jetpack Beta
Option 2. Jurassic Ninja - available for logged-in A12s🚀 Launch a JN site with this branch 🚀 ℹ️ Install this Tampermonkey script to get more options. Build info:
Note: the build is updated when a new commit is pushed to this PR. |
Size Change: +1.47 kB (0%) Total Size: 1.39 MB
ℹ️ View Unchanged
|
@pierorocca Can you please confirm that the adjustment in size looks fine in the screenshots provided? I set the PMME font to be at most 85% of the Payment Method Label size in both blocks and shortcode checkout. Other flows (product details, cart) are unchanged. |
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.
I've left a few small comments for tiny things that could be tweaked. In general, I thought the changes here look pretty tidy: I think the appearance looks perfect on the blocks checkout, though I think the font-size of the PMME text on the shortcode checkout appeared a little small.
Here's what the Figma designs look like.
Figma
Blocks
Label: 15px, PMME: 13px, 87% difference
Shortcode
Label: 16px, PMME: 13px, 81% difference
Following the sizes described in the designs, I can see how you arrived at using an 85% multiplier for the font shrinkage. However, here's what I noticed testing across a few themes locally.
Storefront
Blocks
Shortcode
AFAICT, for both blocks and shortcode the target font-size is 16px and the computed PMME font-size becomes 13.6px, but for some reason this appears slightly differently on the shortcode checkout.
Now here come a few of my favourite uglier themes for further comparison.
Twenty Twenty-One
Blocks
Here the label text is supposedly 20px and our PMME text becomes 17px.
Shortcode
The label font here is 16px and our PMME font is 13.6px.
Deli
Blocks
13.712 px ➡️ 11.6552px
Shortcode
This looks OK too actually, though maybe touching on the smaller side
13.712 px ➡️ 11.6552px
The more that I stare at this, I think that the main issue might be the difference in spacing on the shortcode checkout.
One other thing to note that in the designs, while the shortcode text has a greater font-size differential, the shortcode design actually uses a different font from the blocks design. Perhaps this explains the difference in appearance.
Anyways, let me know your thoughts and whether you noticed any of these sizing differences yourself or am I just seeing things? Do we maybe need to use a different PMME_RELATIVE_TEXT_SIZE
constant for the blocks and shortcode checkouts? Let me know what you think. 🙏
CC @pierorocca
client/checkout/upe-styles/index.js
Outdated
globalRules.fontSizeBase = ensureFontSizeSmallerThan( | ||
selectors.pmmeRelativeTextSizeSelector, | ||
paragraphRules.fontSize, | ||
PMME_RELATIVE_TEXT_SIZE // The font size of the payment method messaging should be 85% or less of the target font size. |
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.
Just a thought, but it's a little strange to pass a constant as a function parameter. This function isn't used anywhere else and there isn't yet really an indication that it might be, so this parameter seems a little superfluous, since it will never change.
Perhaps we can remove the parameter and just use this constant inside the body of ensureFontSizeSmallerThan()
instead? 🤔
client/checkout/upe-styles/index.js
Outdated
@@ -527,9 +565,18 @@ export const getAppearance = ( elementsLocation, forWooPay = false ) => { | |||
colorBackground: backgroundColor, | |||
colorText: paragraphRules.color, | |||
fontFamily: paragraphRules.fontFamily, | |||
// fontSizeBase: transformFontSize( paragraphRules.fontSize, 0.9 ), |
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.
// fontSizeBase: transformFontSize( paragraphRules.fontSize, 0.9 ), |
I presume we don't need this anymore.
@FangedParakeet I have updated the default size percentage to Another thing that I noticed is that even if now both Classic and Blocks checkout were using 14px in the PMME containers, the text looked a bit larger in blocks. I found out that we were not sending the font rules to the container, so it used a font (Helvetica Neue) that looked larger than the actual font (Source Sans Pro). After fixing that, now the fields look like this (Storefront):
|
One of the secondary goals here is to have the fonts across the entire checkout form aligned. So if the payments secondary font size ends up being computed differently to the secondary font size in order summary component or elsewhere on Checkout that would be a problem and an inconsistency. |
@elizaan36 @ralucaStan could you please sanity check this against pdFofs-2xh-p2? Thank you. |
@danielmx-dev Could you share before and after images after this change. Thank you. |
The Before remains unchanged as the one shown in the description. Blocks
Shortcode
|
I see that now thanks Daniel. |
Fixes #9818
Changes proposed in this Pull Request
The size of PMME inside the Payment labels should be smaller than the label that contains the payment method name.
Testing instructions
Regression Testing
No changes in size should be visible for PMME in Product Details and Cart pages.
npm run changelog
to add a changelog file, choosepatch
to leave it empty if the change is not significant. You can add multiple changelog files in one PR by running this command a few times.Post merge