feat(themes): add FontAwesome theme by layershifter · Pull Request #1337 · microsoft/fluent-ui-react

layershifter · 2019-05-14T14:32:21Z

BREAKING CHANGES

FontAwesome icons is not a part of Teams theme more consider to use SVG icons.

FontAwesome styles were extracted to a separate theme which can be merged with the base for example.

import { mergeThemes, themes } from '@stardust-ui/react'

const myTheme = mergeThemes(themes.base, themes.fontAwesome)

Adds separate roots for font and SVG icons, so styles can be configured independently now without conditions in styles:

root: () => {} // will be applied to both types
fontRoot: () => {} // will be applied only for font icons
svgRoot: () => {} // will be applied only for SVG icons

codecov · 2019-05-14T17:30:35Z

Codecov Report

Merging #1337 into master will increase coverage by 0.19%.
The diff coverage is 40%.

@@            Coverage Diff             @@
##           master    #1337      +/-   ##
==========================================
+ Coverage    72.2%   72.39%   +0.19%     
==========================================
  Files         769      770       +1     
  Lines        5777     5749      -28     
  Branches     1688     1680       -8     
==========================================
- Hits         4171     4162       -9     
+ Misses       1600     1581      -19     
  Partials        6        6

Impacted Files	Coverage Δ
...t/src/themes/base/components/Icon/iconVariables.ts	`0% <ø> (ø)`	⬆️
packages/react/src/themes/teams/index.tsx	`90.9% <ø> (ø)`
...eams/components/RadioGroup/radioGroupItemStyles.ts	`18.18% <ø> (ø)`	⬆️
...c/themes/font-awesome/components/Icon/iconNames.ts	`100% <ø> (ø)`
.../src/themes/teams/components/Icon/iconVariables.ts	`0% <0%> (ø)`	⬆️
...eact/src/themes/base/components/Icon/iconStyles.ts	`17.39% <0%> (-2.61%)`	⬇️
...react/src/components/RadioGroup/RadioGroupItem.tsx	`100% <100%> (ø)`	⬆️
packages/react/src/components/Icon/Icon.tsx	`100% <100%> (+33.33%)`	⬆️
...react/src/themes/base/components/Icon/iconNames.ts	`75% <100%> (ø)`
...teams/components/Icon/svg/icons/stardustCircle.tsx	`100% <100%> (ø)`
... and 4 more

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 f0635e4...3ec7f3d. Read the comment docs.

…rdust-ui/react into feat/fa-theme

layershifter · 2019-05-16T14:06:16Z

docs/src/app.tsx

        <Provider
          as={React.Fragment}
-          theme={mergeThemes(themes[themeName], {
+          theme={mergeThemes(themes.fontAwesome, themes[themeName], {


We're using FA on docs, but we don't to have this theme in any other even in base

layershifter · 2019-05-16T14:07:08Z

docs/src/components/ComponentDoc/ComponentExample/ComponentExample.tsx

              render={this.renderElement}
              renderHtml={showCode}
              resolver={importResolver}
+              themeName={themeName}


This will allow us to access themeName in example components

layershifter · 2019-05-16T14:07:37Z

docs/src/examples/components/Avatar/Variations/AvatarExampleImageCustomization.shorthand.tsx

        // and otherwise produces bad results when border is applied compared to "normal" image
        <Icon
-          name="user"
+          name="lock"


Teams theme doesn't have this icon and font icons will not be supported inside of it

I am a bit confuse here. We are changing only this icon because Teams' theme doesn't support font icons? What about all other examples like: 'chess rook', 'book' etc. Are we planning to move all examples for now to reflect the Teams' theme icon names?

We are changing only this icon because Teams' theme doesn't support font icons?

This slot in attachment requires an additional styling for font icons that is not present, so yes.

layershifter · 2019-05-16T14:08:43Z

packages/react/src/components/Icon/Icon.tsx

+          styles: {
+            ...(isSvgIcon ? styles.svgRoot : styles.fontRoot),
+            ...styles.root,
+          },


I have to use styles to avoid collisions in generated classes names... The other option is to use generateCSS() directly, but I will need to get renderer from somewhere

layershifter · 2019-05-16T14:12:36Z

packages/react/src/themes/base/components/Icon/iconStyles.ts

+      color: v.disabledColor,
+    }),
+  }),
+  fontRoot: ({ props: p, variables: v, theme: t }): ICSSInJSStyle => {


Now we have separate roots for SVG and font icons that allows to apply overrides without conditions

I like that we have two things separated 👍

layershifter · 2019-05-16T14:13:19Z

packages/react/src/themes/base/components/Icon/iconVariables.ts

+  mediumSize: string
+  largeSize: string
+  largerSize: string
+  largestSize: string


Size values where moved to variables

Not sure that these size values are general enough for base theme...

Base theme should support size prop properly and we should avoid nesting in variables. Do you have better proposal? 🐱

I agree that we should have variables and use this, I was saying about the size values (which now reflect the Teams theme) If you think these values are general and should be defined as this, then I am okay

…rdust-ui/react into feat/fa-theme # Conflicts: # CHANGELOG.md

mnajdova · 2019-05-16T15:13:26Z

packages/react/src/themes/font-awesome/index.ts

+export default {
+  icons,
+  componentStyles,
+} as ThemeInput


Can we not expose this theme to the users? Everything is breaking if I choose this one on the Icon's docs. I don't see any benefit for the clients to explore this theme..

docs/src/examples/components/Icon/Usage/IconSetExample.shorthand.tsx

packages/react/src/components/Icon/Icon.tsx

packages/react/src/themes/base/components/Icon/iconStyles.ts

mnajdova · 2019-05-16T15:26:16Z

packages/react/src/themes/font-awesome/components/Icon/iconStyles.ts

+
+const iconStyles: ComponentSlotStylesInput<IconProps, IconVariables> = {
+  fontRoot: (): ICSSInJSStyle => ({
+    fontWeight: 900, // required for the fontAwesome to render


It's great that the only required override is actually related to the font awesome icons. I would really like the same thing to be applicable for svgs :)

mnajdova · 2019-05-16T15:28:27Z

packages/react/src/themes/teams/components/RadioGroup/radioGroupItemStyles.ts

    // overrides from icon styles
    backgroundColor: 'transparent',
    boxShadow: 'none',
+    boxSizing: 'border-box',


Shouldn't this be applied by default?? We are using only svg icons and they already have border-box

Based on offline discussion we decided to add a new icon and use it.

mnajdova · 2019-05-16T15:29:27Z

packages/react/src/themes/teams/components/Icon/iconStyles.ts

+    return {
+      display: 'inline-block',
+      backgroundColor: v.backgroundColor,
+      boxSizing: 'border-box',


No need for specifying boxSizing: 'border-box'. Teams theme users only svg icons, and there is no prev definition of 'content-box' (it is for font icons in base theme, not svg)

…rdust-ui/react into feat/fa-theme # Conflicts: # CHANGELOG.md

feat(themes): add FontAwesome theme

cc9befa

layershifter added the 🚧 WIP label May 14, 2019

fix visual regressions

75de2bb

layershifter added 13 commits May 15, 2019 10:09

Merge branches 'feat/fa-theme' and 'master' of https://github.com/sta…

2d89de6

…rdust-ui/react into feat/fa-theme

fix more regressions

04a0fed

fix more regressions

62a807c

fix regression

acbc177

Merge branches 'feat/fa-theme' and 'master' of https://github.com/sta…

82b3934

…rdust-ui/react into feat/fa-theme

restructure styles

83a7732

update examples

aa3d935

update styles

6931faa

fix regressions

4fe3fa8

fix conflicts

4f36475

fixing regressions

55d9490

fixing regressions

816ac38

fixing regressions

c0fc01a

layershifter marked this pull request as ready for review May 16, 2019 14:05

layershifter requested review from dvdzkwsk, kuzhelov, levithomason, miroslavstastny and mnajdova as code owners May 16, 2019 14:05

layershifter commented May 16, 2019

View reviewed changes

add changelog entry

c231413

Merge branches 'feat/fa-theme' and 'master' of https://github.com/sta…

9b19f65

…rdust-ui/react into feat/fa-theme # Conflicts: # CHANGELOG.md

layershifter added 🧰 fix Introduces fix for broken behavior. 🚀 ready for review and removed 🚧 WIP labels May 16, 2019

mnajdova reviewed May 16, 2019

View reviewed changes

docs/src/examples/components/Icon/Usage/IconSetExample.shorthand.tsx Show resolved Hide resolved

mnajdova reviewed May 16, 2019

View reviewed changes

packages/react/src/components/Icon/Icon.tsx Show resolved Hide resolved

mnajdova reviewed May 16, 2019

View reviewed changes

packages/react/src/themes/base/components/Icon/iconStyles.ts Show resolved Hide resolved

mnajdova reviewed May 16, 2019

View reviewed changes

layershifter added 7 commits May 17, 2019 11:02

wip

d4d8917

finish icon

e70b839

update example

a13e630

update circle

aa17a05

update circle again

ebfe10a

move circle to circle

9ce2080

Merge branches 'feat/fa-theme' and 'master' of https://github.com/sta…

66453f0

…rdust-ui/react into feat/fa-theme # Conflicts: # CHANGELOG.md

mnajdova approved these changes May 17, 2019

View reviewed changes

layershifter and others added 2 commits May 20, 2019 10:20

Merge branch 'master' into feat/fa-theme

8a97ad1

Update CHANGELOG.md

3ec7f3d

layershifter merged commit bbace32 into master May 20, 2019

delete-merged-branch bot deleted the feat/fa-theme branch May 20, 2019 08:59

layershifter mentioned this pull request May 22, 2019

Memory: Remove fontawesome icons #1284

Closed

layershifter mentioned this pull request May 31, 2019

fix(Icon): revert changes with different roots #1435

Merged

Conversation

layershifter commented May 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

BREAKING CHANGES

Uh oh!

codecov bot commented May 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

layershifter May 17, 2019 • 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!

layershifter May 16, 2019 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

layershifter commented May 14, 2019 •

edited

Loading

codecov bot commented May 14, 2019 •

edited

Loading

layershifter May 17, 2019 •

edited

Loading

layershifter May 16, 2019 •

edited

Loading