UI tweaks #2: font sizes and consequences #3678
Conversation
kmcgrady
left a comment
kmcgrady
left a comment
There was a problem hiding this comment.
The implications are pretty easy to grok. Been testing with a few examples, and the font is slightly bigger.
The main concern surrounds the px vs rem usage (see comments).
| $small-font-size: 0.875em !default; | ||
| $sub-sup-font-size: 0.75em !default; | ||
| $small-font-size: 14px !default; // Same as fontSizeSmall in typography.ts | ||
| $sub-sup-font-size: 12px !default; // Same as fontSizeTwoSmall in typography.ts |
There was a problem hiding this comment.
Same here. This is originally from Bootstrap's variable files, but if the base is set in the HTML tag, shouldn't be easily an rem
There was a problem hiding this comment.
but if the base is set in the HTML tag, shouldn't be easily an rem
These are used in reboot.scss to set the size of <small> and <sub>/<sub>. If I change them to rem they'll have to be something like 0.777778rem and 0.666667rem. Is that OK with you?
By the way, I'd love for us to move these variables from SASS to CSS --custom-properties. This way we could share them across CSS and TS files directly.
There was a problem hiding this comment.
Yea. That could work. There's a couple Sass functions that we use. We can try to remove them or recreate them not as a function.
There was a problem hiding this comment.
Want me to do this in this PR? Or is that a future improvement?
kmcgrady
left a comment
kmcgrady
left a comment
There was a problem hiding this comment.
Something weird happens for the buttons for number input here. Not sure if it's intentional.
Great catch! Fixed in c916a53 by removing the hard-coded height and using flex instead. |
|
Looks great here. TIL the |
tvst
force-pushed
the
feature/ui-2-fontsizes
branch
from
c916a53 to
569e708
Compare
|
Good to merge onto the feature branch, then? |
|
@tvst Good to merge (save for the merge conflict). |
tvst
force-pushed
the
feature/ui-2-fontsizes
branch
from
dc50a3a to
f2f2e1f
Compare
|
Fixed. I also noticed I had missed two things:
PTAL one last time before I merge! |
|
@tvst Looks good. Can merge with the exception of that conflict needing addressed. |
tvst
force-pushed
the
feature/ui-2-fontsizes
branch
from
f2f2e1f to
e563676
Compare
(Breaking up #3642 into multiple PRs)
NOTE: Screenshot tests coming after first round of reviews. Don't want to generate and re-generate as more reviews come in 😅
NOTE #2: Note the base branch for this PR. I'm basing it on the previous UI tweaks PR so the diff is easier to read. The idea is to merge in order.