fix: refactor approach for px-to-rem conversions by kuzhelov · Pull Request #371 · microsoft/fluent-ui-react

kuzhelov · 2018-10-18T11:43:22Z

Blocked by

decision on functionality we see to be introduced by pxToRem

This PR suggests an approach to pxToRem that would solve the following problems we are currently struggling with:

ensure that there is no 'race condition' when global font size styles are applied (styles that change default font size for the page)
allow each theme to define the value that should be used as a base one for pxToRem calculations - as each theme might be opinionated about this value (e.g. Semantic is built with base value of 10px being considered, Teams - with value of 14px)
eliminate necessity for theme to change global base font size value - as this loaded theme might have its own "opinion" about what the base size should be for these conversions, and with two or more themes trying to fight for this global font size value the unexpected rendering results might happen (like the ones that we see now for Semantic and Teams themes being loaded on the same page)

Fixes #394

codecov · 2018-10-18T11:48:05Z

Codecov Report

❗ No coverage uploaded for pull request base (master@eeba7a0). Click here to learn what that means.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master     #371   +/-   ##
=========================================
  Coverage          ?   91.84%           
=========================================
  Files             ?       41           
  Lines             ?     1337           
  Branches          ?      193           
=========================================
  Hits              ?     1228           
  Misses            ?      105           
  Partials          ?        4

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update eeba7a0...9531bb2. Read the comment docs.

mnajdova

I like the changes. I have one additional question. Are we exporting from stardust the pxToRem service so that it can be used when defining the theme? This is very important for the consumers.

bmdalex

couple of comments but agree with general approach

bmdalex · 2018-10-19T15:01:30Z

src/themes/teams/utils/index.ts

@@ -0,0 +1,3 @@
+import { pxToRem } from '../../../lib'
+
+export const teamsPxToRem = (sizeInPx: number) => pxToRem(sizeInPx, 14)


shouldn't 14 magic number be a property of the theme object, maybe defaultFontSize?

Util

Since this util is scoped to the teams theme, I propose not prefixing it with teams. In fact, since the base util will need to use a required key from the theme (html font size), it seems this util should remain a base util provided by Stardust itself so all consumers can rely on the correct font size being used.

Theme based html font size

Yep, bump. Reminder that this is required as the value needs to be configurable from the Provider. This enables the same component to be used in apps with different html font sizes.

I would propose using htmlFontSize as the top level key, however. Since, this is explicitly what the value is. It is also used by the same name for the same purpose in Material's theme config.

bmdalex · 2018-10-19T15:02:54Z

src/themes/teams/siteVariables.ts

-//
-// VARIABLES
-//
-export const htmlFontSize = '10px' // what 1rem represents


shouldn't we make this 14 and use it in the new teamsPxToRem method?

levithomason · 2018-10-22T15:59:16Z

docs/src/app.tsx

 import Router from './routes'

-const semanticStyleOverrides = {
+const semanticStylesOverrideTheme = {


levithomason · 2018-10-22T16:02:02Z

src/themes/teams/utils/index.ts

@@ -0,0 +1,3 @@
+import { pxToRem } from '../../../lib'
+
+export const teamsPxToRem = (sizeInPx: number) => pxToRem(sizeInPx, 14)


Util

Since this util is scoped to the teams theme, I propose not prefixing it with teams. In fact, since the base util will need to use a required key from the theme (html font size), it seems this util should remain a base util provided by Stardust itself so all consumers can rely on the correct font size being used.

Theme based html font size

Yep, bump. Reminder that this is required as the value needs to be configurable from the Provider. This enables the same component to be used in apps with different html font sizes.

I would propose using htmlFontSize as the top level key, however. Since, this is explicitly what the value is. It is also used by the same name for the same purpose in Material's theme config.

src/lib/fontSizeUtility.ts

layershifter · 2018-11-06T15:57:41Z

I've checked Button docs page, we run pxToRem() there 1313 times. We can reach up to 5x/27x boost with memoization, a little perf test that confirms this: https://jsperf.com/stardust-ui-pxtorem

This is out of scope of this PR, just a note.

kuzhelov · 2018-11-20T00:17:47Z

@layershifter, absolutely agree - actually, this topic was discussed long ago, essentially the same strategy has been suggested (memoise data that was used as part of styles calculation - there is definitely a good opportunity opportunity to utilize (styles, theme)-based caching). The reason it wasn't implemented is that we've agreed on the following plan:

reach correct and flexible implementation first (which should be proved by real app's usage)
obtain perf metrics (scaffold infrastructure for that)
and after that optimise :)

kuzhelov · 2018-11-20T02:24:27Z

Understand pxToRem() semantics

This section, probably, is the most important one, as it lays down foundation for all the following conclusions made in this post.

Definition

pxToRem(pxValue) => remValue,

where remValue, when rendered, is equal to pxValue if htmlFontSize equals to theme's fontSize.

Simple case: `htmlFontSize` equals to theme's font size

theme font size (defined by theme spec) is 14px
button width is 28px
pxToRem(28px) => 2rem
- rendered as 28px if htmlFontSize = themeSpecFontSize = 14px

Advanced case: `htmlFontSize` is different from theme's default

Then we should 'tell' this fact to theme. Consider that for aforementioned example we have htmlFontSize equal to 18px, value that is different from theme's default (14px). In that case we would expect the following:

let theme know that page's default htmlFontSize is 18px
- then pxToRem(28px) => someRemValue
rendered: someRemValue * htmlFontSize = someRemValue * 18px = 28px

API proposal

<Provider theme={{
  ...
  fontFaces: ...,

  // optional - 16px is used if not specified. 
  // Should correspond to initial <html>'s font size used on page.
  htmlFontSize: 10
}}>

Transform formula

The following formula should be used to do all the necessary conversions (trivial to infer from examples above):

pxToRem(pxValue) = themeSpecFontSize (14px) * pxValue / actualHtmlFontSize (18px)

Applied to example's case:

pxToRem(28px) => themeSpecFontSize (14px) * pxValue (28px) / actualHtmlFontSize (18px) => someRemValue (1.55rem)
rendered: someRemValue (1.55rem) * htmlFontSize (18px) = 28px

Theme nesting case

// theme is adjusted to htmlFontSize=10px
// note, value will be 16px if not explicitly specified
// - normally should be defined for topmost Provider only, 
// - as its value SHOULD correspond to page's htmlFontSize.
<Provider theme={{ htmlFontSize: 10, ... }}>
   ... value from topmost provider is passed via context ..

    { /* will use context-defined value (10px) */ }
   <Provider theme={{ ... }}>
      ...
   </Provider>
  
   { /* this Provider's subtree will be rendered with rem sizes as if
    14px would be a value of htmlFontSize.
    - not sure when normally it might be necessary,
    - just maybe in case if relative scales of two themes need to be adjusted. */ }
   <Provider theme={{ htmlFontSize: 14 }}>
     ...
   </Provider>
</Provider>

Thoughts

I really think that biggest problem of this functionality is how not intuitive its effect might be for a client.

Simple example: if I will increase font size value for Provider's htmlFontSize, then, as effect, I will see my components rendered at smaller size - this behavior is reverse of what we expect from HTML's font size. This has lead to couple of issues for Material UI folks who have exactly the same functionality introduced, most loud one: mui/material-ui#10579

We should really do our best by:

use better name for Provider's prop - pageFontSize, some other?
properly deliver semantics of pxToRem via docs (in one sentence)
providing good example page (where elements will change size according to theme's one).

Why not consume `htmlFontSize` from DOM?

Note that we shouldn't read HTML font size from DOM as part of our logic (and let user specify this prop manually instead) - this is necessary because otherwise we will either:

introduce race condition if htmlFontSize is rendered from DOM once topmost Provider is rendered: it could happen after user will manually scale page's font size (for accessibility purposes)
remove scaling ability from page - in case if htmlFontSize will always be read from DOM on render.

Apparently, Material UI folks have same thoughts on this matter: mui/material-ui#10687:

I would encourage people to use the typography.fontSize option as applying a font size on the element prevents the accessibility feature of the browser to kick in.

layershifter · 2018-12-06T10:34:26Z

src/themes/teams/utils/index.ts

@@ -0,0 +1,3 @@
+import { pxToRem as basePxToRem } from '../../../lib'
+
+export const pxToRem = (sizeInPx: number) => basePxToRem(sizeInPx, 14)


Can we avoid there a magic number and define 14 as a separate const? 😺

Suggested change

export const pxToRem = (sizeInPx: number) => basePxToRem(sizeInPx, 14)

/* Teams use 14px as a base font size. */

const baseFontSize = 14

export const pxToRem = (sizeInPx: number) => basePxToRem(sizeInPx, baseFontSize)

layershifter

Looks like a great simplification, I definitely like it, nice job 👍

layershifter · 2018-12-06T10:37:26Z

@kuzhelov seems we have a visual regressions, please check them before merge. And we should do something with naming there, because now we have two pxToRem().

…into fix/px-to-rem

kuzhelov · 2018-12-06T20:29:14Z

docs/src/app.tsx

-          theme={mergeThemes(semanticStyleOverrides, themes[themeName], {
-            // adjust Teams' theme to Semantic UI's font size scheme
-            siteVariables: {
-              htmlFontSize: '14px',


this now should be defined as <html> font size in docs' top-level index.ejs - to eliminate screen test regressions

agreed to proceed with this set of changes, as this is already considerable one. Note that changes provided doesn't introduce any regressions - and, at the same time, fix the problem of rendered page being unable to adjust font size with HTML font size changes. User-defined font sizes functionality will be introduced in the subsequent PR

This reverts commit 9785c8a.

* Revert "fix: refactor approach for px-to-rem conversions (#371)" This reverts commit 9785c8a. * additional changes necessary to revert * update changelog

refactor approach to pxToRem

6be4821

kuzhelov added the 🚧 WIP label Oct 18, 2018

kuzhelov requested review from dvdzkwsk, levithomason, miroslavstastny and mnajdova as code owners October 18, 2018 11:43

kuzhelov added 🚀 ready for review and removed 🚧 WIP labels Oct 18, 2018

mnajdova approved these changes Oct 19, 2018

View reviewed changes

bmdalex approved these changes Oct 19, 2018

View reviewed changes

levithomason previously requested changes Oct 22, 2018

View reviewed changes

levithomason added needs author changes Author needs to implement changes before merge and removed 🚀 ready for review labels Oct 22, 2018

hughreeling suggested changes Nov 1, 2018

View reviewed changes

src/lib/fontSizeUtility.ts Outdated Show resolved Hide resolved

EUROPE\hueland added 5 commits November 6, 2018 15:41

fixing merge

126609a

Allowing negative values

8608b9b

Fixing merge

bc3d23d

Changing name back to pxToRem

152a65d

Fixing test

07c7996

hughreeling approved these changes Nov 6, 2018

View reviewed changes

kuzhelov added 💥 blocked 🚧 WIP and removed needs author changes Author needs to implement changes before merge 💥 blocked 🚧 WIP labels Nov 20, 2018

hughreeling mentioned this pull request Nov 30, 2018

pxToRem() doesn't support negative values #394

Closed

kuzhelov added ready for merge and removed 💥 blocked labels Dec 4, 2018

kuzhelov and others added 3 commits December 4, 2018 15:29

Merge branch 'master' into fix/px-to-rem

aa30a3d

merge recent changes

157e7e9

Merge branch 'master' into fix/px-to-rem

47ea370

kuzhelov requested a review from layershifter as a code owner December 6, 2018 09:24

layershifter reviewed Dec 6, 2018

View reviewed changes

layershifter approved these changes Dec 6, 2018

View reviewed changes

layershifter added needs author feedback Author's opinion is asked and removed ready for merge labels Dec 6, 2018

kuzhelov and others added 7 commits December 6, 2018 20:01

Merge branch 'master' into fix/px-to-rem

81db169

set html font size explicitly

4bb7eed

Merge branch 'fix/px-to-rem' of https://github.com/stardust-ui/react …

6ee3115

…into fix/px-to-rem

set default font size 16px

bc8e67b

html font size 14px

00f1998

introduce Teams theme font size constant

a43e7cc

update changelog

9531bb2

kuzhelov commented Dec 6, 2018

View reviewed changes

kuzhelov merged commit 9785c8a into master Dec 6, 2018

kuzhelov deleted the fix/px-to-rem branch December 6, 2018 20:40

kuzhelov pushed a commit that referenced this pull request Jan 10, 2019

Revert "fix: refactor approach for px-to-rem conversions (#371)"

0d23ac9

This reverts commit 9785c8a.

kuzhelov mentioned this pull request Jan 10, 2019

fix(px-to-rem): temporary remove recent changes #700

Merged

1 task

kuzhelov added a commit that referenced this pull request Jan 10, 2019

fix(px-to-rem): temporary remove recent changes (#700)

910709e

* Revert "fix: refactor approach for px-to-rem conversions (#371)" This reverts commit 9785c8a. * additional changes necessary to revert * update changelog

		@@ -0,0 +1,3 @@
		import { pxToRem } from '../../../lib'

		export const teamsPxToRem = (sizeInPx: number) => pxToRem(sizeInPx, 14)

		@@ -0,0 +1,3 @@
		import { pxToRem as basePxToRem } from '../../../lib'

		export const pxToRem = (sizeInPx: number) => basePxToRem(sizeInPx, 14)

-export const pxToRem = (sizeInPx: number) => basePxToRem(sizeInPx, 14)
+/* Teams use 14px as a base font size. */
+const baseFontSize = 14
+export const pxToRem = (sizeInPx: number) => basePxToRem(sizeInPx, baseFontSize)

Conversation

kuzhelov commented Oct 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Blocked by

Uh oh!

codecov bot commented Oct 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

mnajdova left a comment

Choose a reason for hiding this comment

Uh oh!

bmdalex left a comment

Choose a reason for hiding this comment

Uh oh!

bmdalex Oct 19, 2018

Choose a reason for hiding this comment

Uh oh!

levithomason Oct 22, 2018

Choose a reason for hiding this comment

Util

Theme based html font size

Uh oh!

bmdalex Oct 19, 2018

Choose a reason for hiding this comment

Uh oh!

levithomason Oct 22, 2018

Choose a reason for hiding this comment

Uh oh!

levithomason Oct 22, 2018

Choose a reason for hiding this comment

Util

Theme based html font size

Uh oh!

Uh oh!

layershifter commented Nov 6, 2018

Uh oh!

kuzhelov commented Nov 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kuzhelov commented Nov 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Understand pxToRem() semantics

Definition

Simple case: htmlFontSize equals to theme's font size

Advanced case: htmlFontSize is different from theme's default

API proposal

Transform formula

Applied to example's case:

Theme nesting case

Thoughts

Why not consume htmlFontSize from DOM?

Uh oh!

layershifter Dec 6, 2018

Choose a reason for hiding this comment

Uh oh!

layershifter left a comment

Choose a reason for hiding this comment

Uh oh!

layershifter commented Dec 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kuzhelov Dec 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

kuzhelov commented Oct 18, 2018 •

edited

Loading

codecov bot commented Oct 18, 2018 •

edited

Loading

kuzhelov commented Nov 20, 2018 •

edited

Loading

kuzhelov commented Nov 20, 2018 •

edited

Loading

Simple case: `htmlFontSize` equals to theme's font size

Advanced case: `htmlFontSize` is different from theme's default

Why not consume `htmlFontSize` from DOM?

layershifter commented Dec 6, 2018 •

edited

Loading

kuzhelov Dec 6, 2018 •

edited

Loading