-
Notifications
You must be signed in to change notification settings - Fork 327
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
Calculate font-sizes in rem #858
Conversation
excellent stuff. |
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.
Looks good to me 🙂 Just left a couple of small comments.
I enabled $govuk-typography-use-rem
and tested whether the font size adjusts in the following browsers:
✅ Chrome 67.0 (set font size in Settings)
✅ Firefox 61 (set default font in Preferences)
✅ Safari 11.1 Mac OS X (set minimum font size in Advanced > Accessibility)
✅ Samsung Internet on Android Galaxy S8 (set font size in Settings)
✅ Chrome on Android Galaxy S8 (set font size in Settings)
These don't work but this is a limitation with iOS 😞 not our code:
🚫 Chrome iPhone 5s (set Larger text in General > Accessibility)
🚫 Safari iPhone 5s (set Larger text in General > Accessibility)
@@ -2,6 +2,36 @@ | |||
/// @group settings/typography | |||
//// | |||
|
|||
/// Whether or not to define font sizes in rem. This is currently off by |
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.
It might be good to have an explanation here to say that enabling rem
measurements improves accessibility as users can adjust the font size at system level. Just in case someone's reading this and is not sure if using rem
is something they should do.
src/tools/_px-to-rem.scss
Outdated
/// Convert pixels to rem | ||
/// | ||
/// The $govuk-root-font-size must match the font-size on your root (html) | ||
/// element |
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.
Could we add "See src/settings/_typography-responsive.scss
for more information."
Looks like there might be ways to enable this but I think that's definitely something for the future. |
Automatically calculate font-sizes in rem by adding a new setting $govuk-root-font-size, which should match the font-size on the html (root) element.
Enabling rem for everyone would make this a breaking change for anyone who is using alphagov/govuk_template or otherwise setting the font-size on their HTML element to anything other than 16px - without them setting `$govuk-root-font-size` this would make their typography the wrong size. We can introduce it as a feature now and enable it by default in the next major release, whilst at the same time changing how the ‘compatibility’ flags work and updating the $govuk-root-font-size setting to set sensible defaults in both cases.
Doing this makes it breaks users font-size preferences in some browsers (such as Chrome) – see twbs/bootstrap#19460 for more details. We should instead _assume_ the root font-size is 16px - the point being that if it’s not then the text should adjust accordingly.
@hannalaakso are you happy with this now? |
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.
Looks great 🎉
Also, good spot on the dynamic type 👍
This PR supercedes #839
Optionally enable automatically calculating font-sizes in rem by adding new settings
$govuk-typography-use-rem
and$govuk-root-font-size
, which should match the font-size on the html (root) element.In order to avoid this being a breaking change for users who use a different font-size on their root element (for example anyone using alphagov/govuk_template) this feature is going to be disabled by default for now. We'll enable it for everyone by default in the next major release.