fix(Text): refactor variables to match size prop
#762
Conversation
layershifter
requested review from
dvdzkwsk,
kuzhelov,
levithomason,
miroslavstastny and
mnajdova
as code owners
| <Text key={color} color={color}> | ||
| {_.startCase(color)} | ||
| </Text> | ||
| <React.Fragment key={color}> |
There was a problem hiding this comment.
Fixes key warning 👍
| @@ -8,11 +8,13 @@ export { colors, contextualColors, emphasisColors, naturalColors, colorScheme } | |||
| // FONT SIZES | |||
| // | |||
| export const fontSizes = { | |||
| smallest: '0.8rem', | |||
There was a problem hiding this comment.
while it seems to be logical, the counterpoint is that we, generally, will 'close' the interval of possible values in terms of extensibility - especially when we are talking about base theme.
The problem is that, with this being, essentially, a finite set, we cannot guarantee that client's scenario won't require additional sizes added to the set - and, in that case, we cannot ensure that client will be able to extend base theme and introduce, say, 10 (and not 7, as prescribed) font sizes with meaningful names .
So, because of that I am a bit reluctant to introduce this change now.
| fontSizeExtraSmall: siteVariables.fontSizes.smaller, | ||
| fontLineHeightExtraSmall: siteVariables.lineHeightExtraSmall, | ||
| fontSizeSmallest: siteVariables.fontSizes.smallest, | ||
| fontLineHeightSmallest: siteVariables.lineHeightExtraSmall, |
There was a problem hiding this comment.
seems that this one wasn't replaced
| @@ -8,11 +8,13 @@ export { colors, contextualColors, emphasisColors, naturalColors, colorScheme } | |||
| // FONT SIZES | |||
| // | |||
| export const fontSizes = { | |||
| smallest: '0.8rem', | |||
| smaller: '1rem', | |||
| small: '1.2rem', | |||
| medium: '1.4rem', | |||
| large: '1.8rem', | |||
There was a problem hiding this comment.
frankly, to me it is not absolutely intuitive whether large or larger is bigger while just having a markup:
<Button size='larger' />Further thoughts
It would be much better to have some range where each value's name would immediately deliver insights on what is the name of next larger item and next smaller one, like, say, in the following case:
xx-small // or x2-small, to reduce amount of x's
x-small
medium
large
x-large
xx-largeAmongst other things it would be great if this range would be extendable, so that client's theme may easily provide additional sizes to it while maintaining the overall order of values.
With this order being established, we might be able to decently handle scenarios when one theme, with wider range of sizes, is switched on another one where this range is smaller:
// original theme
const fontSizes = {
xx-small
x-small
medium
large
x-large
xx-large
}
// theme to switch on - note, no values for xx-large/small
const fontSizes = {
x-small
medium
large
x-large
}The idea here is just to allow next theme to choose nearest size to the one that is requested (so, for xx-large new theme will use its x-large) - and this is pretty much the same what is happening with fonts in Web (both speaking of sizes and weights).
To conclude
This was just one of the approaches we might think about - definitely, there may be lots of other ones. At the same time, if we would like to provide client with an intuitive range of values, as well as to support theme switching between two that have different set of sizes defined, we would need to satisfy the following conditions:
- names of values suggest their order
- it is possible to infer next closest value from the name of each one
- there is an extendable range of values
There was a problem hiding this comment.
The sizing scheme we decided to go with for now is: smallest, smaller, small, medium, large, larger, largest.
For themes that may want 5 sizes, just have the largest/smallest sizes use the same values as larger/smaller to limit them.
There was a problem hiding this comment.
thanks for these references and clarifications!
| fontLineHeightSmallest: siteVariables.lineHeightExtraSmall, | ||
|
|
||
| fontSizeSmaller: siteVariables.fontSizes.smaller, | ||
| fontLineHeightSmaller: siteVariables.lineHeightSmaller, |
There was a problem hiding this comment.
just to be sure - thus, speaking of the lineHeight range, we will have values from smaller to small, right?
…rdust-ui/react into fix/text-size # Conflicts: # CHANGELOG.md
Fixes #454.
TODO