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

Configure default font settings as Public Sans #264

Merged
merged 3 commits into from
Oct 29, 2021
Merged

Conversation

aduth
Copy link
Member

@aduth aduth commented Oct 22, 2021

Why: To reconcile differences between the brochure site and design system, update design system to reflect current typographic preferences.

Consuming projects can still upgrade and choose to temporarily bypass this change by reconfiguring it to the old setting on a per-project basis.

**Why**: To reconcile differences between the brochure site and design system, update design system to reflect current typographic preferences.

Consuming projects can still upgrade and choose to temporarily bypass this change by reconfiguring it to the old setting on a per-project basis.
Copy link
Contributor

@zachmargolis zachmargolis left a comment

Choose a reason for hiding this comment

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

LGTM, this seems like a big jump (even though it has an escape hook to set the font back via variable)... WDYT of making it default to the current way and then changing the default in a bigger version bump?

Or since we're realistically the only consumers of this repo, it's prob fine

@aduth
Copy link
Member Author

aduth commented Oct 25, 2021

this seems like a big jump (even though it has an escape hook to set the font back via variable)... WDYT of making it default to the current way and then changing the default in a bigger version bump?

Good question. It's something I've contended with in how best to version changes in this project, since technically most any visual change could be counted as backwards-incompatible by some interpretations. To avoid major version bumps for basically any change, my personal view is to consider these sorts of things under the realm of "this is now the new status quo", and as long as it doesn't require any updates to markup / usage on the consuming end, it's a minor version. I feel the same way here, even though in practice we may need to temporarily use the escape hatch for at least the IdP. I generally feel this is a safe change that wouldn't require changes on the consuming end, or at least the changes which come up are likely to be the sort I'd classify as existing bugs (with text reflow in particular) that are only incidentally revealed by a change in font family.

@zachmargolis
Copy link
Contributor

my personal view is to consider these sorts of things under the realm of "this is now the new status quo", and as long as it doesn't require any updates to markup / usage on the consuming end, it's a minor version

that seems like a good enough reason for me! So "you don't need to touch the HTML" = minor/patch and "you definitely need to change the HTML too" = major?

@aduth
Copy link
Member Author

aduth commented Oct 25, 2021

that seems like a good enough reason for me! So "you don't need to touch the HTML" = minor/patch and "you definitely need to change the HTML too" = major?

Yeah, that sounds right to me. Which I think is where this one rides close to the line, since ideally it won't require any changes, but it might in some specific brittle implementations. For the IdP, we're tracking these at LG-4792 and LG-4791.

@aduth aduth merged commit 9212899 into main Oct 29, 2021
@aduth aduth deleted the aduth-public-sans branch October 29, 2021 13:57
@aduth aduth mentioned this pull request Jan 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants