Skip to content
This repository has been archived by the owner on Sep 11, 2024. It is now read-only.

Make all 'font-size's and 'line-height's rem #4305

Merged

Conversation

JorikSchellekens
Copy link
Contributor

Font size of the whole app would ideally be controlled by a single
value. This value is currently hard coded using the :root CSS selector.
It is the intention to make this value configurable within riot. In the
interim all font-sizes have been converted to rem by the simple process
of regex. Replacing px values with their equivalent rem values assuming
a font size of 15px and then rounded to three decimal places, which was
the base at the time of this transformation.

This commit doesn't address any scaling issues. I thought it better to
land this unwieldy, mechanical, invisible change before the others
otherwise the pr would be impossible to review thoroughly.

Font size of the whole app would ideally be controlled by a single
value. This value is currently hard coded using the :root CSS selector.
It is the intention to make this value configurable within riot. In the
interim all font-sizes have been converted to rem by the simple process
of regex. Replacing px values with their equivalent rem values assuming
a font size of 15px and then rounded to three decimal places, which was
the base at the time of this transformation.

I'm expecting another commit cleaning up rem values but I thought it
best to leave that to review.

This commit doesn't address any scaling issues. I thought it better to
land this unwieldy, mechanical, invisible change before the others
otherwise the pr would be impossible to review thoroughly.
@JorikSchellekens JorikSchellekens requested a review from a team March 30, 2020 17:25
Copy link
Member

@turt2live turt2live left a comment

Choose a reason for hiding this comment

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

Is design going to start giving us mockups using rem values instead of pixel values? If not, it would be good to introduce some variables like $font-14px: 0.933rem (or real css variables) so we don't need to do the math ourselves every time. Might be good to have the variables anyways, actually.

Also please don't forget to add cards to the workflow board :) I've gone ahead and done it this time (https://github.com/vector-im/riot-web/projects/11#card-35448990) though sometimes stuff will be missed because it hadn't made it to the workflow board yet.

@nadonomy
Copy link
Contributor

Hey @JorikSchellekens, btw you can request reviews from @matrix-org/design and they automatically get put on the design teams radar. :)

For visual changes, we'd normally ask for an ad hoc build to be thrown up somewhere to interact with, but given the px/rem tests we did with @bwindels yesterday these specific changes look reasonable to me.

Is design going to start giving us mockups using rem values instead of pixel values?

Design tools aren't this flexible sadly.

@JorikSchellekens
Copy link
Contributor Author

Alright, I'll reformat everything to use 'pixel variables'.

Thanks for the tips!

It's become obvious that these random floating points everywhere
are unwieldy. Now they're all in one place with some fairly logical
variable names which will help out in design->implementation phase.
@JorikSchellekens
Copy link
Contributor Author

Just put all the variables into one place

@JorikSchellekens
Copy link
Contributor Author

Working on getting a test riot hosted for this and in general

res/css/_font-sizes.scss Outdated Show resolved Hide resolved
res/css/rethemendex.sh Outdated Show resolved Hide resolved
@jryans
Copy link
Collaborator

jryans commented Apr 2, 2020

@JorikSchellekens Is this ready to merge or did you want another look?

@JorikSchellekens JorikSchellekens merged commit 3ed457e into matrix-org:develop Apr 2, 2020
@JorikSchellekens JorikSchellekens deleted the joriks/font-scaling branch April 3, 2020 10:01
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants