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
[CYS on Core] Ensure the size of fonts is not affected by the font family changes #44424
Conversation
Test Results SummaryCommit SHA: 15a1b4e
To view the full API test report, click here. To view the full E2E test report, click here. To view all test reports, visit the WooCommerce Test Reports Dashboard. |
Hi @chihsuan, @gigitux, @nefeline, @woocommerce/woo-fse Apart from reviewing the code changes, please make sure to review the testing instructions as well. You can follow this guide to find out what good testing instructions should look like: |
user.styles, | ||
variation.styles | ||
), | ||
styles: variation.styles, |
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.
Hey @albarin Thanks for working on this! 👍
Unlike the GB site editor, I use mergeBaseAndUserConfigs
here because we've split style variations into font and color parts. We need to preserve the color changes when updating the font and vice versa.
I tested this PR on JN, and you can see when I change the color and then update the font, the color gets reset. I think this is a bit tricky. Perhaps a way to fix it is to create a custom merge function. 🤔
Screen.Recording.2024-02-08.at.11.27.15.mov
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.
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'm pre-approving as everything is working as described (great job!), but I'll defer to @chihsuan for the code review, as he's way more versed than I at this area of the codebase :)
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 looks good. Thanks! 💯
Submission Review Guidelines:
Changes proposed in this Pull Request:
This PR resets the typography settings from the user styles before applying the new ones to avoid influencing one another.
Closes #44259
How to test the changes in this Pull Request:
Using the WooCommerce Testing Instructions Guide, include your detailed testing instructions:
WooCommerce Beta Tester
plugin is installed and activated (available on this monorepo)./wp-admin/tools.php?page=woocommerce-admin-test-helper
and enablecustomize-store
feature flag.wp-admin/admin.php?page=wc-admin&path=/customize-store
Change fonts
.Montserrat + Arvo
.Albert Sans + Lora
, the size is the same.Rubik + Inter
and you'll see the font is bigger.Albert Sans + Lora
and check the font is the same size compared to the initialAlbert Sans + Lora
pairing (clicking onMontserrat + Arvo
should not influence the size ofAlbert Sans + Lora
).Change the color palette
and select a new one.Change fonts
and select a new one.Changelog entry
Significance
Type
Message
CYS - Ensure the size of fonts is not affected by a font family change.
Comment