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

BREAKING(Text): change font size scheme and Text size API #214

Merged
merged 11 commits into from
Sep 27, 2018

Conversation

codepretty
Copy link
Collaborator

@codepretty codepretty commented Sep 11, 2018

Before

<Text size="xs" />
<Text size="sm" />
<Text size="md" />
<Text size="lg" />
<Text size="xl" />
<Text size="2x" />
<Text size="3x" />
<Text size="4x" />

After

<Text size="smallest" />
<Text size="smaller" />
<Text size="small" />
<Text size="medium" />
<Text size="large" />
<Text size="larger" />
<Text size="largest" />

  • Updated sizes to be smallest, smaller, small, medium, large, larger, largest
  • Updated doc examples to show font sizes that are supported by the theme

Was

  • showed hard-coded font sizes that were the same for all themes
    image

Now

  • shows only the font sizes that are defined in a theme's siteVariables
    image

@codepretty codepretty changed the title [WIP] Show font size doc examples based on theme supported font sizes fix(Text): Show font size doc examples based on theme supported font sizes Sep 11, 2018
@codepretty codepretty added question Further information is requested, concerns that require additional thought are raised 🚀 ready for review and removed 🚀 ready for review labels Sep 11, 2018
@codepretty codepretty changed the title fix(Text): Show font size doc examples based on theme supported font sizes fix(Text): [WIP] Show font size doc examples based on theme supported font sizes Sep 12, 2018
Copy link
Member

@miroslavstastny miroslavstastny left a comment

Choose a reason for hiding this comment

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

By this change, you are defining a contract for all themes that they must define their sizes in fontSizes.
How do you plan to enforce it? It should be at least added to ISiteVariables

render={({ siteVariables }) => {
return _.map(siteVariables.fontSizes, (value, key) => (
<div key={key}>
<Text size={key} content="Dicta voluptatum dolorem." />
Copy link
Member

Choose a reason for hiding this comment

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

I would also add the size to the content so I can immediately see the size names in docsite.

@codecov
Copy link

codecov bot commented Sep 26, 2018

Codecov Report

Merging #214 into master will decrease coverage by 0.07%.
The diff coverage is 33.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #214      +/-   ##
==========================================
- Coverage   91.45%   91.37%   -0.08%     
==========================================
  Files          63       63              
  Lines        1158     1160       +2     
  Branches      153      176      +23     
==========================================
+ Hits         1059     1060       +1     
- Misses         95       96       +1     
  Partials        4        4
Impacted Files Coverage Δ
src/components/Text/Text.tsx 100% <ø> (ø) ⬆️
src/components/Chat/ChatMessage.tsx 93.61% <ø> (ø) ⬆️
src/components/Provider/Provider.tsx 60% <33.33%> (-0.47%) ⬇️

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 b252a8a...8168760. Read the comment docs.

@codepretty codepretty changed the title fix(Text): [WIP] Show font size doc examples based on theme supported font sizes fix(Text): Update font size doc examples based on theme supported font sizes Sep 26, 2018
@codepretty codepretty added 🚀 ready for review and removed question Further information is requested, concerns that require additional thought are raised labels Sep 26, 2018
Copy link
Member

@levithomason levithomason left a comment

Choose a reason for hiding this comment

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

We will consider how to enforce this with typings later. This topic also warrants an RFC on /specifications for how we deal with font sizes and component sizes at the framework API level. The changes here are the result of many conversations and ideas with Fabric, teams around MS, and the open source. We chose a middle ground path for sizes for now. We are open to future changes.

@levithomason levithomason changed the title fix(Text): Update font size doc examples based on theme supported font sizes BREAKING(Text): change font size scheme and Text size API Sep 27, 2018
@levithomason levithomason merged commit ea881ee into master Sep 27, 2018
@levithomason levithomason deleted the fix/theme-font-enums branch September 27, 2018 20:08
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants