Skip to content
This repository has been archived by the owner on Mar 4, 2020. It is now read-only.

fix(Text): refactor variables to match size prop #762

Merged
merged 10 commits into from
Jan 24, 2019
3 changes: 3 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,9 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm

## [Unreleased]

### BREAKING CHANGES
- Update variable names in themes, add missing sizes @layershifter ([#762](https://github.com/stardust-ui/react/pull/762))

<!--------------------------------[ v0.18.0 ]------------------------------- -->
## [v0.18.0](https://github.com/stardust-ui/react/tree/v0.18.0) (2019-01-24)
[Compare changes](https://github.com/stardust-ui/react/compare/v0.17.0...v0.18.0)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,10 @@ const TextExampleColor = () => (
<ProviderConsumer
render={({ siteVariables: { emphasisColors, naturalColors } }) =>
_.keys({ ...emphasisColors, ...naturalColors }).map(color => (
<>
<Text key={color} color={color} content={_.startCase(color)} />
<React.Fragment key={color}>
<Text color={color} content={_.startCase(color)} />
<br />
</>
</React.Fragment>
))
}
/>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,12 +6,10 @@ const TextExampleColor = () => (
<ProviderConsumer
render={({ siteVariables: { emphasisColors, naturalColors } }) =>
_.keys({ ...emphasisColors, ...naturalColors }).map(color => (
<>
<Text key={color} color={color}>
{_.startCase(color)}
</Text>
<React.Fragment key={color}>
Copy link
Member Author

Choose a reason for hiding this comment

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

Fixes key warning 👍

<Text color={color}>{_.startCase(color)}</Text>
<br />
</>
</React.Fragment>
))
}
/>
Expand Down
16 changes: 12 additions & 4 deletions src/themes/base/components/Text/textStyles.ts
Original file line number Diff line number Diff line change
Expand Up @@ -53,9 +53,13 @@ export default {
fontWeight: v.fontWeightBold,
}),

...(size === 'smallest' && {
fontSize: v.fontSizeSmallest,
lineHeight: v.fontLineHeightSmallest,
}),
...(size === 'smaller' && {
fontSize: v.fontSizeExtraSmall,
lineHeight: v.fontLineHeightExtraSmall,
fontSize: v.fontSizeSmaller,
lineHeight: v.fontLineHeightSmaller,
}),
...(size === 'small' && {
fontSize: v.fontSizeSmall,
Expand All @@ -70,8 +74,12 @@ export default {
lineHeight: v.fontLineHeightLarge,
}),
...(size === 'larger' && {
fontSize: v.fontSizeExtraLarge,
lineHeight: v.fontLineHeightExtraLarge,
fontSize: v.fontSizeLarger,
lineHeight: v.fontLineHeightLarger,
}),
...(size === 'largest' && {
fontSize: v.fontSizeLargest,
lineHeight: v.fontLineHeightLargest,
}),
}
},
Expand Down
30 changes: 20 additions & 10 deletions src/themes/base/components/Text/textVariables.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,17 +13,21 @@ export interface TextVariables {
errorColor: string
successColor: string

fontSizeExtraSmall: string
fontSizeSmallest: string
fontSizeSmaller: string
fontSizeSmall: string
fontSizeMedium: string
fontSizeLarge: string
fontSizeExtraLarge: string
fontSizeLarger: string
fontSizeLargest: string

fontLineHeightExtraSmall: number
fontLineHeightSmallest: number
fontLineHeightSmaller: number
fontLineHeightSmall: number
fontLineHeightMedium: number
fontLineHeightLarge: number
fontLineHeightExtraLarge: number
fontLineHeightLarger: number
fontLineHeightLargest: number

fontWeightLight: number
fontWeightSemilight: number
Expand All @@ -47,20 +51,26 @@ export default (siteVariables): TextVariables => {
errorColor: siteVariables.colors.red[500],
successColor: siteVariables.colors.green[500],

fontSizeExtraSmall: siteVariables.fontSizes.smaller,
fontLineHeightExtraSmall: siteVariables.lineHeightExtraSmall,
fontSizeSmallest: siteVariables.fontSizes.smallest,
fontLineHeightSmallest: siteVariables.lineHeightSmallest,

fontSizeSmaller: siteVariables.fontSizes.smaller,
fontLineHeightSmaller: siteVariables.lineHeightSmaller,
Copy link
Contributor

Choose a reason for hiding this comment

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

just to be sure - thus, speaking of the lineHeight range, we will have values from smaller to small, right?


fontSizeSmall: siteVariables.fontSizes.small,
fontLineHeightSmall: siteVariables.lineHeightSmall,

fontSizeMedium: siteVariables.fontSizes.medium,
fontLineHeightMedium: siteVariables.lineHeightBase,
fontLineHeightMedium: siteVariables.lineHeightMedium,

fontSizeLarge: siteVariables.fontSizes.large,
fontLineHeightLarge: siteVariables.lineHeightSmall,
fontLineHeightLarge: siteVariables.lineHeightLarge,

fontSizeLarger: siteVariables.fontSizes.larger,
fontLineHeightLarger: siteVariables.lineHeightLarger,

fontSizeExtraLarge: siteVariables.fontSizes.larger,
fontLineHeightExtraLarge: siteVariables.lineHeightSmall,
fontSizeLargest: siteVariables.fontSizes.largest,
fontLineHeightLargest: siteVariables.lineHeightLargest,

fontWeightLight: siteVariables.fontWeightLight,
fontWeightSemilight: siteVariables.fontWeightSemilight,
Expand Down
10 changes: 8 additions & 2 deletions src/themes/base/siteVariables.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,11 +8,13 @@ export { colors, contextualColors, emphasisColors, naturalColors, colorScheme }
// FONT SIZES
//
export const fontSizes = {
smallest: '0.8rem',
Copy link
Contributor

@kuzhelov kuzhelov Jan 23, 2019

Choose a reason for hiding this comment

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

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.

smaller: '1rem',
small: '1.2rem',
medium: '1.4rem',
large: '1.8rem',
Copy link
Contributor

Choose a reason for hiding this comment

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

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-large

Amongst 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

Copy link
Member Author

Choose a reason for hiding this comment

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

The sizing scheme we decided to go with for now is: smallest, smaller, small, medium, large, larger, largest.

#640

For themes that may want 5 sizes, just have the largest/smallest sizes use the same values as larger/smaller to limit them.

#136 (comment)

Copy link
Contributor

Choose a reason for hiding this comment

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

thanks for these references and clarifications!

larger: '2.4rem',
largest: '3rem',
}

//
Expand All @@ -27,6 +29,10 @@ export const fontWeightBold = 700
//
// LINE HEIGHTS
//
export const lineHeightBase = 1.4286
export const lineHeightSmallest = 1.2
export const lineHeightSmaller = 1.2
export const lineHeightSmall = 1.3333
export const lineHeightExtraSmall = 1.2
export const lineHeightMedium = 1.4286
export const lineHeightLarge = 1.3333
export const lineHeightLarger = 1.3333
export const lineHeightLargest = 1.3333
2 changes: 1 addition & 1 deletion src/themes/teams/components/Menu/menuVariables.ts
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,6 @@ export default (siteVars: any): MenuVariables => {

disabledColor: siteVars.gray06,

lineHeightBase: siteVars.lineHeightBase,
lineHeightBase: siteVars.lineHeightMedium,
}
}
8 changes: 5 additions & 3 deletions src/themes/teams/siteVariables.ts
Original file line number Diff line number Diff line change
Expand Up @@ -83,9 +83,11 @@ export const fontWeightBold = 700
//
// LINE HEIGHTS
//
export const lineHeightBase = 1.4286
export const lineHeightSmaller = 1.2
export const lineHeightSmall = 1.3333
export const lineHeightExtraSmall = 1.2
export const lineHeightMedium = 1.4286
export const lineHeightLarge = 1.3333
export const lineHeightLarger = 1.3333

//
// SEMANTIC ASSIGNMENTS
Expand All @@ -97,4 +99,4 @@ export const bodyFontFamily =
export const bodyFontSize = '1.4rem'
export const bodyBackground = white
export const bodyColor = black
export const bodyLineHeight = lineHeightBase
export const bodyLineHeight = lineHeightMedium