-
-
Notifications
You must be signed in to change notification settings - Fork 814
Support browser font scaling #11972
Support browser font scaling #11972
Conversation
richvdh
left a comment
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.
looks generally sensible to me, modulo the comments here.
Also, CI is sad and there is a conflict.
| // Needs to be string for things like '17.' without | ||
| // trailing 0s. |
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.
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)
| "baseFontSizeV2": { | ||
| displayName: _td("settings|appearance|font_size"), | ||
| supportedLevels: [SettingLevel.DEVICE], | ||
| default: FontWatcher.DEFAULT_SIZE, |
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.
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.
| const cssFontSize = | ||
| fontSize === FontWatcher.DEFAULT_SIZE | ||
| ? "100%" | ||
| : `calc(100% + ${toPx(fontSize - FontWatcher.DEFAULT_SIZE)})`; |
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.
For a bit DRYer:
| 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)
|
We went with a different approach: #12246 |
Fixes element-hq/element-web#26669
100%, and relative to 100% wherebaseFontSizeV2is not == default. This change means EW will now respect browser font sizes and scale text accordingly.useCustomFontSizesetting and inputdisplayFunconSlidercomponentChecklist
Here's what your changelog entry will look like:
✨ Features