The Wayback Machine - https://web.archive.org/web/20200714041820/https://github.com/matrix-org/matrix-react-sdk/pull/4424
Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Font scaling settings and slider #4424

Merged

Conversation

@JorikSchellekens
Copy link
Member

JorikSchellekens commented Apr 16, 2020 •

Requires vector-im/riot-web#13199
Requires 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:

  1. It migrates the theme settings to a style tab
  2. It implements a font control system in the style tab which is hidden behind labs

image

Font size 18
image

Font size 14
image

@JorikSchellekens JorikSchellekens requested review from nadonomy and matrix-org/riot-web Apr 16, 2020
@turt2live turt2live requested review from turt2live and removed request for matrix-org/riot-web Apr 17, 2020
Copy link
Member

turt2live left a comment

Overall the actual change looks good, just some comments on the project structure.

res/css/structures/_TagPanel.scss Outdated Show resolved Hide resolved
src/components/structures/FontSlider.js Outdated Show resolved Hide resolved
src/components/structures/FontSlider.js Outdated Show resolved Hide resolved
src/components/structures/FontSlider.js Outdated Show resolved Hide resolved
src/components/structures/FontSlider.js Outdated Show resolved Hide resolved
src/settings/Settings.js Outdated Show resolved Hide resolved
src/theme.js Outdated Show resolved Hide resolved
@JorikSchellekens JorikSchellekens force-pushed the JorikSchellekens:joriks/font-scaling-slider branch 2 times, most recently from 075b47f to dc57590 Apr 21, 2020
@nadonomy
Copy link
Member

nadonomy commented Apr 22, 2020 •

Hey @JorikSchellekens, on:

  1. Use only one of the input methods
  2. Use both methods but have only one available at a time according to a toggle
  3. Use both methods which mutually update each other as displayed in the screenshots

Let's stick to the designs, as per (2), where:

  • The stepper isn't labelled with numbers
  • There's a toggle to enable custom input
  • When custom input is enabled, the stepper enters a disabled state
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.
Copy link
Member

turt2live left a comment

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;
Comment on lines 266 to 267

This comment has been minimized.

Copy link
@turt2live

turt2live May 4, 2020

Member

we generally prefer all-lowercase variables

This comment has been minimized.

Copy link
@JorikSchellekens

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.

Copy link
@turt2live

turt2live May 6, 2020

Member

yes. We should add a lot of stuff to the linter :D

src/FontWatcher.js Outdated Show resolved Hide resolved
src/FontWatcher.js Outdated Show resolved Hide resolved
src/components/views/elements/Slider.tsx Outdated Show resolved Hide resolved
src/settings/Settings.js Outdated Show resolved Hide resolved
@nadonomy
Copy link
Member

nadonomy commented May 6, 2020 •

  • Test if first clicks on the slider are more performant without animations
  • R&D letting browsers know text sizes could change in advance to avoid poor performance on first clicks (worth pairing on with Bruno)
  • Nad & Rok to look again at interactions between slider & custom— switching between the 2 feels strange, clicking on the slider steps is hard, font size values are abstract (consider using 100% as default and letting users input relative to that), solve slider not scaling
  • Nad to get better clarity on values (up to 20px is safe)
  • Nad to dogfood + make a list of obviously broken stuff
@JorikSchellekens
Copy link
Member Author

JorikSchellekens commented May 6, 2020

Thanks @turt2live for the lints. They should all be cleaned up now. Wondering if we can add lowercase css variables to the linter

@JorikSchellekens JorikSchellekens requested a review from turt2live May 7, 2020
@turt2live
Copy link
Member

turt2live commented May 7, 2020

@JorikSchellekens this has conflicts and doesn't look like the last review was actually resolved? (no pushed commits)

@JorikSchellekens
Copy link
Member Author

JorikSchellekens commented May 7, 2020

Woops sorry @turt2live , pushed them now

@JorikSchellekens
Copy link
Member Author

JorikSchellekens commented May 13, 2020 •

Definitely seems like a smoother experience without the animations. The screen jump as a result of font size change isn't overly disturbing

@JorikSchellekens
Copy link
Member Author

JorikSchellekens commented May 13, 2020

I'm not getting poor performance, and without the animations I think it's no longer a problem.

- also fiddles the font size numbers
Copy link
Member

turt2live left a comment

Otherwise lgtm - thanks!

Apologies for the delays on these reviews :(

src/settings/Settings.js Outdated Show resolved Hide resolved
src/settings/Settings.js Outdated Show resolved Hide resolved
@JorikSchellekens
Copy link
Member Author

JorikSchellekens commented May 20, 2020

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.

Copy link
Member

turt2live left a comment

otherwise lgtm, assuming the tests can be made to pass

src/FontWatcher.js Outdated Show resolved Hide resolved
@JorikSchellekens JorikSchellekens merged commit d95d019 into matrix-org:develop May 20, 2020
11 checks passed
11 checks passed
buildkite/matrix-react-sdk Build #6599 passed (6 minutes, 57 seconds)
Details
buildkite/matrix-react-sdk/chains-end-to-end-tests Passed (6 minutes, 49 seconds)
Details
buildkite/matrix-react-sdk/eslint-js-lint Passed (2 minutes, 12 seconds)
Details
buildkite/matrix-react-sdk/eslint-ts-lint Passed (44 seconds)
Details
buildkite/matrix-react-sdk/eslint-types-lint Passed (49 seconds)
Details
buildkite/matrix-react-sdk/globe-with-meridians-i18n Passed (51 seconds)
Details
buildkite/matrix-react-sdk/hammer-and-wrench-build Passed (1 minute, 28 seconds)
Details
buildkite/matrix-react-sdk/jest-tests Passed (2 minutes, 53 seconds)
Details
buildkite/matrix-react-sdk/pipeline Passed (5 seconds)
Details
buildkite/matrix-react-sdk/stylelint-style-lint Passed (41 seconds)
Details
buildkite/matrix-react-sdk/wrench-riot-tests Passed (3 minutes, 53 seconds)
Details
@8go

This comment was marked as off-topic.

@jryans

This comment was marked as off-topic.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

5 participants
You can’t perform that action at this time.