Fix font-size scaling for text-related CSS properties by using rem instead of fixed px values; deprecate $root-font-size
#1169
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mattxwang
changed the title
rem instead of px in certain SASS unitsrem instead of px for all text related CSS properties
Closed
Closed
|
A quick light ping for @just-the-docs/reviewers; if anybody has capacity to take a look at this change, that would be great! It will finally resolve all warnings on build, and I'll move on to some other requested work - e.g. automatic theme switching. |
cking0987
approved these changes
Jun 8, 2023
There was a problem hiding this comment.
This looks great. I double-checked the math on all the new REM values. Removing the functions.scss file cleans things up nicely. The site builds perfectly (no more depreciation warning about division). Just two comments:
- It's worth mentioning that you decided to go with the non-breaking option to leave the $root-font-size variable. So I think this PR is no longer a breaking change.
- The left padding for blockquotes was changed from 15px to 1rem. This is the only conversion that wasn't exactly equivalent at 16px=1rem. But I don't think it matters— they still look like blockquotes.
|
Thanks for the quick review @cking0987, and sorry to leave you hanging! Great opportunity to cut a new release and address one of our recent headaches. |
mattxwang
changed the title
rem instead of px for all text related CSS propertiesrem; deprecate $root-font-size
mattxwang
changed the title
rem; deprecate $root-font-sizerem instead of fixed px values; deprecate $root-font-size
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR replaces all uses of
pxin relation to font size (opposed to borders, spacing, etc.) with the equivalentremvalue when the body font size is16px. The intention is to better scale the website when the user changes the font size for<body>(often done for accessibility reasons).This PR is technically a breaking change, though it's a minor one (see subheading below). I'm putting this up so that we can discuss it as a community.
(technically closes #1088 and fixes #1073, but let's see if we end up merging this)
mechanics
To do this, I systematically went through every
pxvalue for all.scssfiles. Then, I deleted theremfunction, the_functions.scssfile (that was the only function there), and removed the import fromsupport.scss. A nice side effect of this is that we no longer perform any SASS division.The only remaining uses of
pxare for either:hr,blockquotedecorative spacing)$root-font-size(see below)The only pixel value change in this PR is the
padding-leftforblockquote, which I've changed from15pxto1rem(which is16pxin the "stock" theme).deprecating
$root-font-sizeThere's a SCSS variable called
$root-font-size. It is used in two places:rem()function.site-titlewhen printing (i.e. a@printstyle)The changes I listed above let us ignore the first case. The second case seems like it has the intention of matching the body font size, so I replaced it with
1rem.We can choose to leave the variable in (in case others use it in custom code - which I'm sure that some do) and leave a deprecation notice, or just remove it now. I'm leaning towards the former, which is less disruptive.
how users would upgrade
This is a breaking change of some sorts, but the change is very straightforward for users:
$root-font-size, they need to do nothing; this PR is a no-op.$root-font-sizefont-sizeofbodywith the appropriatepxvalue$root-font-sizewith1rem(find-and-replace works here)