Skip to content
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

MWPW-165992 Fix Chart Font Family #3685

Merged
merged 2 commits into from
Feb 21, 2025

Conversation

meganthecoder
Copy link
Contributor

  • Updates chart font family to match milo body font
  • Fixes issue where incorrect font is sometimes seen on Safari

Resolves: MWPW-165992

Test URLs:

Copy link
Contributor

aem-code-sync bot commented Feb 13, 2025

Page Scores Audits Google
📱 /drafts/methomas/chart-two-up?martech=off PERFORMANCE A11Y SEO BEST PRACTICES SI FCP LCP TBT CLS PSI
🖥️ /drafts/methomas/chart-two-up?martech=off PERFORMANCE A11Y SEO BEST PRACTICES SI FCP LCP TBT CLS PSI

Copy link
Contributor

This pull request is not passing all required checks. Please see this discussion for information on how to get all checks passing. Inconsistent checks can be manually retried. If a test absolutely can not pass for a good reason, please add a comment with an explanation to the PR.

@@ -23,7 +23,7 @@ export default (deviceSize) => {
const axisColor = '#767676';
const options = {
textStyle: {
fontFamily: 'Adobe Clean',
fontFamily: '"Adobe Clean", adobe-clean, "Trebuchet MS", sans-serif',
Copy link
Contributor

@narcis-radu narcis-radu Feb 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we really need to set a font family here? Ideally we want to inherit from body, which has the font family defined.
Screenshot 2025-02-17 at 12 12 13
Screenshot 2025-02-17 at 12 12 45

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The chart library will add a font family if you don't set any so inheriting from body won't work.

Copy link
Contributor

@narcis-radu narcis-radu Feb 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@meganthecoder - using inherit should do the trick
Screenshot 2025-02-20 at 17 02 56

Copy link
Contributor

@robert-bogos robert-bogos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the failing NALA tests you could try to clear the cached assets with something like:
CURL -si -X POST https://admin.hlx.page/code/robert-bogos/milo/mwpw-142622-project-pep/*

Copy link
Contributor

Reminder to set the Ready for Stage label - to queue this to get merged to stage & production.

@Dli3 Dli3 added verified PR has been E2E tested by a reviewer Ready for Stage labels Feb 21, 2025
@milo-pr-merge milo-pr-merge bot merged commit 67fbb44 into adobecom:stage Feb 21, 2025
20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Ready for Stage verified PR has been E2E tested by a reviewer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants