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

Support browser font scaling #11972

Closed
wants to merge 5 commits into from

Conversation

kerryarchibald
Copy link
Contributor

@kerryarchibald kerryarchibald commented Nov 29, 2023

Fixes element-hq/element-web#26669

  • sets html font size to 100%, and relative to 100% where baseFontSizeV2 is not == default. This change means EW will now respect browser font sizes and scale text accordingly.
  • removes useCustomFontSize setting and input
  • reinstates displayFunc on Slider component
  • displays font slider values as relative to default ("+2")
Screenshot 2023-11-29 at 15 14 07

Checklist

  • Tests written for new code (and old code if feasible)
  • Linter and other CI checks pass
  • Sign-off given on the changes (see CONTRIBUTING.md)

Here's what your changelog entry will look like:

✨ Features

@kerryarchibald kerryarchibald added the T-Enhancement New features, changes in functionality, performance boosts, user-facing improvements label Nov 29, 2023
Copy link
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

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

looks generally sensible to me, modulo the comments here.

Also, CI is sad and there is a conflict.

Comment on lines 33 to 34
// Needs to be string for things like '17.' without
// trailing 0s.
Copy link
Member

Choose a reason for hiding this comment

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

is this still true, particularly given we use a custom formatter? Maybe we can switch to a numeric representation and do a bit less toString and parseInt below?

(It might also be nice to switch it to holding a relative value, to make it closer to the displayed value and what it actually does? But happy for us to declare that out of scope for this PR)

@@ -494,11 +494,6 @@ export const SETTINGS: { [setting: string]: ISetting } = {
default: FontWatcher.DEFAULT_SIZE,
Copy link
Member

Choose a reason for hiding this comment

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

I note there is a reference on line 487 to "16px". It seems like that needs updating, or at least clarifying?

Generally it might be nice to add a little documentation about what baseFontSizeV2 actually does and how its value relates to fontSize on the root element.

Comment on lines +125 to +128
const cssFontSize =
fontSize === FontWatcher.DEFAULT_SIZE
? "100%"
: `calc(100% + ${toPx(fontSize - FontWatcher.DEFAULT_SIZE)})`;
Copy link
Member

Choose a reason for hiding this comment

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

For a bit DRYer:

Suggested change
const cssFontSize =
fontSize === FontWatcher.DEFAULT_SIZE
? "100%"
: `calc(100% + ${toPx(fontSize - FontWatcher.DEFAULT_SIZE)})`;
const relativeSize = fontSize - FontWatcher.DEFAULT_SIZE;
const cssFontSize =
relativeSize === 0
? "100%"
: `calc(100% + ${toPx(relativeSize})`;

(I'd also be inclined to s/${toPx(relativeSize})/${relativeSize}px, but YMMV. I'd also be tempted to get rid of the conditional and just let it be calc(100% + 0px) when it's the default size, but again YMMV)

@robintown
Copy link
Member

We went with a different approach: #12246

@robintown robintown closed this Mar 1, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
T-Enhancement New features, changes in functionality, performance boosts, user-facing improvements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support browser font size settings
3 participants