-
Notifications
You must be signed in to change notification settings - Fork 55
fix: refactor approach for px-to-rem conversions #371
Conversation
Codecov Report
@@ 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.
|
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 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.
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.
couple of comments but agree with general approach
src/themes/teams/utils/index.ts
Outdated
@@ -0,0 +1,3 @@ | |||
import { pxToRem } from '../../../lib' | |||
|
|||
export const teamsPxToRem = (sizeInPx: number) => pxToRem(sizeInPx, 14) |
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.
shouldn't 14
magic number be a property of the theme object, maybe defaultFontSize
?
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.
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/themes/teams/siteVariables.ts
Outdated
// | ||
// VARIABLES | ||
// | ||
export const htmlFontSize = '10px' // what 1rem represents |
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.
shouldn't we make this 14
and use it in the new teamsPxToRem
method?
@@ -6,7 +6,7 @@ import { semanticCssOverrides } from './Style' | |||
import { ThemeContext } from './context/theme-context' | |||
import Router from './routes' | |||
|
|||
const semanticStyleOverrides = { | |||
const semanticStylesOverrideTheme = { |
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.
👍
src/themes/teams/utils/index.ts
Outdated
@@ -0,0 +1,3 @@ | |||
import { pxToRem } from '../../../lib' | |||
|
|||
export const teamsPxToRem = (sizeInPx: number) => pxToRem(sizeInPx, 14) |
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.
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.
I've checked This is out of scope of this PR, just a note. |
@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:
|
Understand pxToRem() semanticsThis section, probably, is the most important one, as it lays down foundation for all the following conclusions made in this post. DefinitionpxToRem( where Simple case:
|
src/themes/teams/utils/index.ts
Outdated
@@ -0,0 +1,3 @@ | |||
import { pxToRem as basePxToRem } from '../../../lib' | |||
|
|||
export const pxToRem = (sizeInPx: number) => basePxToRem(sizeInPx, 14) |
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.
Can we avoid there a magic number and define 14
as a separate const
? 😺
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) |
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 like a great simplification, I definitely like it, nice job 👍
@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 |
…into fix/px-to-rem
theme={mergeThemes(semanticStyleOverrides, themes[themeName], { | ||
// adjust Teams' theme to Semantic UI's font size scheme | ||
siteVariables: { | ||
htmlFontSize: '14px', |
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.
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.
Blocked by
This PR suggests an approach to
pxToRem
that would solve the following problems we are currently struggling with:pxToRem
calculations - as each theme might be opinionated about this value (e.g. Semantic is built with base value of10px
being considered, Teams - with value of14px
)Fixes #394