-
-
Notifications
You must be signed in to change notification settings - Fork 828
Font scaling settings and slider #4424
Font scaling settings and slider #4424
Conversation
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.
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
Compare
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.
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.
largely seems fine from a code perspective - just picking out the style/lint things at this point.
res/themes/light/css/_light.scss
Outdated
$Slider-selection-color: $accent-color; | ||
$Slider-background-color: #c1c9d6; |
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.
we generally prefer all-lowercase variables
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.
Ah, just spotted these change requests. Would it be worth adding this to the linter?
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.
yes. We should add a lot of stuff to the linter :D
|
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. |
- also fiddles the font size numbers
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.
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. |
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.
otherwise lgtm, assuming the tests can be made to pass
Requires element-hq/element-web#13199Requires element-hq/element-web#13352
Fixes element-hq/element-web#3160
Test it out: https://riots.im/adhoc/resize-mania4/
This implements two related things:
Font size 18
Font size 14