Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upFont scaling settings and slider #4424
Conversation
|
Overall the actual change looks good, just some comments on the project structure. |
src/components/views/settings/tabs/user/AppearanceUserSettingsTab.js
Outdated
Show resolved
Hide resolved
075b47f
to
dc57590
|
Hey @JorikSchellekens, on:
Let's stick to the designs, as per (2), where:
|
This was a hard pill to swallow
This fix isn't perfect. Currently the scroll view is slightly smaller than the list of rooms. I think it has something to do with the how the heigh is calculate in js, considering it has some assumptions about the height of each bar and the padding. However room items are the only things which change with respect to the root value. Therefore the item list is actually taller than the computed pixel value of the list converted to rems. I'll look into it.
|
largely seems fine from a code perspective - just picking out the style/lint things at this point. |
| $Slider-selection-color: $accent-color; | ||
| $Slider-background-color: #c1c9d6; |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
JorikSchellekens
May 6, 2020
•
Author
Member
Ah, just spotted these change requests. Would it be worth adding this to the linter?
This comment has been minimized.
This comment has been minimized.
|
|
Thanks @turt2live for the lints. They should all be cleaned up now. Wondering if we can add lowercase css variables to the linter |
|
@JorikSchellekens this has conflicts and doesn't look like the last review was actually resolved? (no pushed commits) |
|
Woops sorry @turt2live , pushed them now |
|
Definitely seems like a smoother experience without the animations. The screen jump as a result of font size change isn't overly disturbing |
|
I'm not getting poor performance, and without the animations I think it's no longer a problem. |
|
Otherwise lgtm - thanks! Apologies for the delays on these reviews :( |
|
We decided to have the slider reactivate itself if it was disabled and it was clicked on. However, with the way the settingtoggle currently works that's a little awkward to do. I'll have a look at clean way of doing this when experimenting with checkboxes under the new appearance tab. |
|
otherwise lgtm, assuming the tests can be made to pass |
d95d019
into
matrix-org:develop


JorikSchellekens commentedApr 16, 2020
•
edited
Requires vector-im/riot-web#13199Requires vector-im/riot-web#13352
Fixes vector-im/riot-web#3160
Test it out: https://riots.im/adhoc/resize-mania4/
This implements two related things:
Font size 18

Font size 14
