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

Swap out 3rd party font for system fonts #1421

Merged
merged 1 commit into from
Apr 24, 2023
Merged

Conversation

mixonic
Copy link
Contributor

@mixonic mixonic commented Apr 22, 2023

Ember CLI Addon Docs should use system fonts by default. System fonts load quickly for users, reduce the site's coupling to 3rd party assets, and make sites privacy-friendly by default.

The Google font previously relied upon would result in Google having the ability to add cookies for that traffic, possibly placing a site out of compliance with EU and other regulations regarding user tracking without consent processes in place.

Here I've replaced (via Addepar/ember-table#983 for the purposes of illustration):

Screenshot of previous font on macOS

Screenshot of previous font on Ubuntu

with

Screenshot of new font on macOS

Screenshot of new font on Ubuntu

Note that the strategy of font stacks is to pick a class of font, so this will render differently on Linux vs. macOS, etc.

I'm not sure what kind of release this warrants, but I'm going to suggest minor. It is not a breaking change, though it is a visual change.

Fixes #1415

Ember CLI Addon Docs should use system fonts by default. System fonts
load quickly for users, reduce the site's coupling to 3rd party assets,
and make sites privacy-friendly by default.

The Google font previously relied upon would result in Google having the
ability to add cookies for that traffic, possibly placing a site out of
compliance with EU and other regulations regardin user tracking without
consent processes in place.
@mixonic mixonic marked this pull request as ready for review April 22, 2023 03:03
mixonic added a commit to Addepar/ember-table that referenced this pull request Apr 22, 2023
To avoid loading fonts from a 3rd party domain, adopt a change in an
upstream PR.

ember-learn/ember-cli-addon-docs#1421
@mixonic
Copy link
Contributor Author

mixonic commented Apr 24, 2023

I'm taking the initiative to land this based on the consensus that any solution was acceptable expressed at #1415.

Ping me if there is anything to follow up on! I'll look into triggering a release in the next day or two.

@mixonic mixonic merged commit 3843a43 into master Apr 24, 2023
'"Palentino Linotype"',
'"URW Palladio L"',
'"P052"',
'serif',

Choose a reason for hiding this comment

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

With serif fallback, it should work pretty much anywhere, but still - maybe its worth adding a windows font to the fallback list ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Per modern font stacks, Palatino Linotype is available on Windows XP and up.

Copy link

@c69-addepar c69-addepar left a comment

Choose a reason for hiding this comment

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

LGTM but small comment

@RobbieTheWagner RobbieTheWagner deleted the mixonic/swap-default-fonts branch April 24, 2023 14:01
@RobbieTheWagner
Copy link
Member

I'm taking the initiative to land this based on the consensus that any solution was acceptable expressed at #1415.

Ping me if there is anything to follow up on! I'll look into triggering a release in the next day or two.

I did not even see this PR before it was merged. It all seems fine to me, but would have been nice to check it out at least before merging.

@mixonic
Copy link
Contributor Author

mixonic commented Apr 25, 2023

@RobbieTheWagner I haven't been able to make a release cut yet, so please feel free to provide feedback and I will address it!

I'm also blocked on making a release until I'm granted permissions on the npm package, if you can unblock that. (@dfreeman was going to help me out)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Not GDPR compliant out of the box
3 participants