feat(theme): add font icons support for themes #243

kuzhelov · 2018-09-17T20:09:04Z

This PR is introduced as one of the steps necessary to address #230.

Overview

Support for font icons is introduced for themes, icon types are generalized (no need to specify svg prop to switch between SVG and font-based icons).

API changes for SVG icons

Before

// SVG icon
<Icon svg name='umbrella' />   

// font icon
<Icon name='user' />

Now

// SVG icon
<Icon name='umbrella' />

// font icon - exactly the same API
<Icon name='user' />

Font icons support for themes

Theme authors are able to introduce custom set of icons, both font-based and SVGs are supported.

// theme.js
const icons = {
  'umbrella': () => <svg>...</svg>,
  'user': { fontFamily: 'Icons', content: '"\\f0152" }
}
...

export default {
  ...
  icons: icons
}

hughreeling

Looks good.

miroslavstastny · 2018-09-19T12:33:02Z

src/lib/renderComponent.tsx

@@ -120,7 +122,8 @@ const renderComponent = <P extends {}>(
      }: IThemeInput | IThemePrepared = {}) => {
        const ElementType = getElementType({ defaultProps }, props)

-        const stateAndProps = { ...state, ...props }
+        const derivedProps = getExtraProps({ icons })


So derivedProps or extraProps?

miroslavstastny · 2018-09-19T12:37:30Z

src/components/Icon/Icon.tsx

-    return this.props.svg
-      ? this.renderSvgIcon(ElementType, icons, classes, rest, accessibility)
-      : this.renderFontIcon(ElementType, classes, rest, accessibility)
+    return this.isFontBased(icons)


I don't like the fact that you do similar calculation to what you've already done in getExtraProps(). Primarily not because of the performance but because for me (as a component developer) it is difficult to understand what the extraProps are good for - they are used to calculate styles, accessibility... why there are not used to render the component?
renderComponent() should receive extraProps as a parameter.

Agreed, we can remove getExtraProps entirely if we check the type of the icon instead. To limit the number of ideas and patterns in the library, I propose we also allow values or functions (as with variables and styles):

const icons = { // value form umbrella: <svg></svg>, user: { fontFamily: 'Icons', content: "\\f0152" }, // function form umbrella: () => <svg></svg>, user: () => ({ fontFamily: 'Icons', content: "\\f0152" }), } React.isValidElement(callable(icons.umbrella)()) ? isSVG : isFONT

This would allow us to pass values to the icon later. It also removes the need for the getExtraProps function.

miroslavstastny · 2018-09-19T12:39:56Z

src/components/Icon/Icon.tsx

@@ -130,10 +136,26 @@ class Icon extends UIComponent<Extendable<IIconProps>, any> {
    )
  }

+  isFontBased(themeIcons: ThemeIcons): boolean {
+    const { name } = this.props


nit: from the function signature, it is not obvious that it checks current icon (in this).
I would change the signature to isFontBased(name, themeIcons)

levithomason · 2018-09-19T16:57:52Z

src/components/Icon/Icon.tsx

+      fontIconFromTheme: isFontBased && icons[this.props.name],
+    }
+  }
+
  renderComponent({ ElementType, classes, rest, accessibility, icons }) {


Propose we pass the entire theme to renderComponent as it is more generic and seems more useful in a wider range of scenarios. Icons alone probably are not that useful to many other components.

codecov · 2018-09-19T19:13:44Z

Codecov Report

Merging #243 into feat/themable-icons will decrease coverage by 0.45%.
The diff coverage is 77.77%.

@@                   Coverage Diff                   @@
##           feat/themable-icons     #243      +/-   ##
=======================================================
- Coverage                91.78%   91.32%   -0.46%     
=======================================================
  Files                       60       60              
  Lines                     1035     1038       +3     
  Branches                   132      154      +22     
=======================================================
- Hits                       950      948       -2     
- Misses                      82       86       +4     
- Partials                     3        4       +1

Impacted Files	Coverage Δ
src/components/Icon/Icon.tsx	`81.48% <77.77%> (-18.52%)`	⬇️

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 72c62eb...f436722. Read the comment docs.

kuzhelov · 2018-09-20T16:19:54Z

agreed with @miroslavstastny to reduce amount of style parts exposed for SVG icons to absolute necessary minimum (instead of path1, path2, path3 ... path9 we have now) before merging changes into master

kuzhelov · 2018-09-20T21:07:06Z

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

@@ -103,7 +98,9 @@ const iconStyles = {
      width: '1em',
      height: '1em',

-      ...(isFontBased ? getFontStyles(font, name, size) : {}),
+      ...(isFontBased


simplify this one

kuzhelov · 2018-09-20T21:11:08Z

types/theme.d.ts

-export type RenderSvgIconFunction = (classes: Classes) => React.ReactNode
+type SvgIconFuncArg = {
+  classes: Classes
+  props: any


props should be eliminated for now

* add SVG icons to teams theme * udpate theme icons api, add example enumerating icons * generalize API for svg and font-based icons * feat(theme): add font icons support for themes (#243) * generalize API for svg and font-based icons * remove 'svg' prop in IconSet example * add mechanism for providing font-based icons in theme * fix UTs * remove getExtraProps method * fix theme icons example in docs * generalize object and function types as icon descriptors * fix unit tests * address review comments * fix lint issues * fix import statement * replace helper methods * remove 'derivedProps' from renderer * remove unused styles for SVG paths * address review comments * fix lint issues * changelog

kuzhelov-ms added 3 commits September 17, 2018 17:58

generalize API for svg and font-based icons

70f9eb5

remove 'svg' prop in IconSet example

0c557e7

add mechanism for providing font-based icons in theme

bb04472

kuzhelov requested review from dvdzkwsk, levithomason, miroslavstastny and mnajdova as code owners September 17, 2018 20:09

kuzhelov added the 🚀 ready for review label Sep 17, 2018

fix UTs

3b07bc8

kuzhelov mentioned this pull request Sep 18, 2018

Theme-defined icons support (including SVG, font-based) #230

Closed

kuzhelov changed the title ~~Add font icons support for themes~~ feat(icons): add font icons support for themes Sep 18, 2018

kuzhelov changed the title ~~feat(icons): add font icons support for themes~~ feat(theme): add font icons support for themes Sep 18, 2018

hughreeling reviewed Sep 19, 2018

View reviewed changes

miroslavstastny reviewed Sep 19, 2018

View reviewed changes

levithomason reviewed Sep 19, 2018

View reviewed changes

remove getExtraProps method

c4a9bb0

kuzhelov-ms added 3 commits September 19, 2018 22:50

fix theme icons example in docs

ebfdb02

generalize object and function types as icon descriptors

f96ac7b

fix unit tests

b2253f9

miroslavstastny approved these changes Sep 20, 2018

View reviewed changes

kuzhelov commented Sep 20, 2018

View reviewed changes

address review comments

f436722

kuzhelov added ready for merge and removed 🚀 ready for review labels Sep 20, 2018

kuzhelov merged commit 835f91d into feat/themable-icons Sep 20, 2018

kuzhelov mentioned this pull request Sep 20, 2018

feat: themable icons #260

Merged

1 task

kuzhelov deleted the feat/generalize-svg-and-font-based-icons-api branch November 19, 2018 15:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(theme): add font icons support for themes #243

feat(theme): add font icons support for themes #243

kuzhelov commented Sep 17, 2018 •

edited

Loading

hughreeling left a comment

miroslavstastny Sep 19, 2018

miroslavstastny Sep 19, 2018

levithomason Sep 19, 2018

miroslavstastny Sep 19, 2018

levithomason Sep 19, 2018

codecov bot commented Sep 19, 2018 •

edited

Loading

kuzhelov commented Sep 20, 2018

kuzhelov Sep 20, 2018

kuzhelov Sep 20, 2018

feat(theme): add font icons support for themes #243

feat(theme): add font icons support for themes #243

Conversation

kuzhelov commented Sep 17, 2018 • edited Loading

Overview

API changes for SVG icons

Before

Now

Font icons support for themes

hughreeling left a comment

Choose a reason for hiding this comment

miroslavstastny Sep 19, 2018

Choose a reason for hiding this comment

miroslavstastny Sep 19, 2018

Choose a reason for hiding this comment

levithomason Sep 19, 2018

Choose a reason for hiding this comment

miroslavstastny Sep 19, 2018

Choose a reason for hiding this comment

levithomason Sep 19, 2018

Choose a reason for hiding this comment

codecov bot commented Sep 19, 2018 • edited Loading

Codecov Report

kuzhelov commented Sep 20, 2018

kuzhelov Sep 20, 2018

Choose a reason for hiding this comment

kuzhelov Sep 20, 2018

Choose a reason for hiding this comment

kuzhelov commented Sep 17, 2018 •

edited

Loading

codecov bot commented Sep 19, 2018 •

edited

Loading