feat(Icon): add support for SVG #50

kuzhelov · 2018-08-06T14:50:40Z

Icon

SVG-based icons are introduced as a default mechanism for creating icon. Font-based icons are still available to the client, but font-based implementation should be explicitly requested:

TODO

API Proposal

Name

By default icon from fonts collection is rendered (FontAwesome, Icons).

// font-based icon (default)
<Icon name="chess rook" />

with HTML:

<span class="ui-icon ...">

Font

If font is provided, an icon with the name is rendered from the collection of glyphs for corresponding font.

// font-based icon, custom font
<Icon font="SomeOtherFont" name="foo" />

SVG

Collection of SVG icons is provided in a separate module, each icon item contains all the necessary DOM to be rendered as child of <svg> element.

To render SVG icon svg property should be set for the Icon component - with name defining the icon to render from SVG icons' bank:

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

results in the following HTML:

<span class="..">
  <svg class="..." viewBox="...">
    <path d="..."></path>
  </svg>
</span>

codecov · 2018-08-06T14:50:56Z

Codecov Report

Merging #50 into master will decrease coverage by 0.45%.
The diff coverage is 69.69%.

@@            Coverage Diff             @@
##           master      #50      +/-   ##
==========================================
- Coverage   87.36%   86.91%   -0.46%     
==========================================
  Files          74       75       +1     
  Lines        1148     1169      +21     
  Branches      217      214       -3     
==========================================
+ Hits         1003     1016      +13     
- Misses        141      147       +6     
- Partials        4        6       +2

Impacted Files	Coverage Δ
src/components/Icon/fontAwesomeIconRules.ts	`80% <ø> (ø)`	⬆️
src/styles/customCSS.ts	`100% <ø> (ø)`	⬆️
src/components/Icon/svgIcons.tsx	`100% <100%> (ø)`
src/components/Icon/Icon.tsx	`82.14% <58.33%> (-17.86%)`	⬇️
src/components/Icon/iconRules.ts	`73.46% <72.22%> (-2.15%)`	⬇️

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 3172970...0a7b2b3. Read the comment docs.

codepretty · 2018-08-06T20:48:40Z

src/components/Icon/svgIcons.tsx

+
+const svgIcons: { [name: string]: ISvgIconSpec } = {
+  umbrella: {
+    viewBox: '0 0 34 34',


Should we have a default value for the viewBox? In Teams, not all, but most of the icons have the same viewBox which is '0 0 32 32'

it should be defined for corresponding module with Teams icons then - we should not introduce any Teams-specific aspects to common logic of Stardust library (also, the 'umbrella' icon is introduced here for demo purposes only).

Here is a way I see this module could be organised to contain Teams SVG icons bank:

// teams-icons.jsx const iconTemplates = { "teamsCreate": (<path d="..." />), "startCall": ( <React.Fragment> <path d="" /> <path d="" /> </React.Fragment>) .... } const commonViewBox = "0 0 32 32" let iconsToExport = {} // 'reduce' will suit better these needs, but idea is the same Object.keys(iconTemplates) .forEach(iconName => { iconsToExport[iconName] = { element: iconTemplates[iconName], viewBox: commonViewBox } }) export default iconsToExport

codepretty · 2018-08-06T20:58:12Z

Does this support more complex SVGs, like SVGs with more paths? Like,

<path d="aaa"></path> <path d="bbb"></path> <path d="ccc"></path> <path d="ddd"></path>

codepretty · 2018-08-06T21:01:02Z

src/components/Icon/svgIcons.tsx

+  umbrella: {
+    viewBox: '0 0 34 34',
+    element: (
+      <path d="M27 14h5c0-1.105-1.119-2-2.5-2s-2.5 0.895-2.5 2v0zM27 14c0-1.105-1.119-2-2.5-2s-2.5 0.895-2.5 2c0-1.105-1.119-2-2.5-2s-2.5 0.895-2.5 2v0 14c0 1.112-0.895 2-2 2-1.112 0-2-0.896-2-2.001v-1.494c0-0.291 0.224-0.505 0.5-0.505 0.268 0 0.5 0.226 0.5 0.505v1.505c0 0.547 0.444 0.991 1 0.991 0.552 0 1-0.451 1-0.991v-14.009c0-1.105-1.119-2-2.5-2s-2.5 0.895-2.5 2c0-1.105-1.119-2-2.5-2s-2.5 0.895-2.5 2c0-1.105-1.119-2-2.5-2s-2.5 0.895-2.5 2c0-5.415 6.671-9.825 15-9.995v-1.506c0-0.283 0.224-0.499 0.5-0.499 0.268 0 0.5 0.224 0.5 0.499v1.506c8.329 0.17 15 4.58 15 9.995h-5z" />


A lot of SVGs can get really long and be pretty complex. I wonder if we might want to consider not inlining them like this in the code as I think it could get really hard to scan and edit. In our project today, an SVG has its own html file which gives the added benefit of formatting

yes, there multiple ways to embrace this use case - essentially, there is no difference for the common Icon's logic on where and how the child content will be provided. There are several HTML-to-JSX transpilers that could be introduced as part of the build logic - those will consume HTML templates as input and create corresponding JS modules for each of the icons. Here is one of them: https://magic.reactjs.net/htmltojsx.htm

At the same time, the same could be achieved much simplier - if I've understood you correctly, the situation is that there is a dedicated HTML template for each SVG icon. In that case svgIcons module could be organised in the following way:

// teamCreateIcon.tsx - provides 'teams-create' icon, only const teamCreateIcon = ( // HTML template goes here - essentially, the same syntax <g> <path d="..."/> ... </g> ) export default teamCreateIcon

// index.js - just assembling all the definitions from multiple files export { default as TeamCreateIcon } from './teamCreateIcon' export { default as StartCallIcon } from './startCallIcon'

Returning to aforementioned question if it is possible to support scenarios where multiple path objects will be provided (instead of one) - sure, it is. Just use React.Fragment as a parent element for this:

// teamCreateIcon.tsx - provides 'teams-create' icon, only const teamCreateIcon = ( // HTML template goes here - essentially, the same syntax <React.Fragment> <path d="..." /> <path d="..." /> ... </React.Fragment> ) export default teamCreateIcon

codepretty · 2018-08-06T21:29:59Z

src/components/Icon/Icon.tsx

+
+    return (
+      <ElementType className={classes.root} {...rest}>
+        <svg className={classes.svg} viewBox={icon && icon.viewBox}>


should we allow optional accessibility props here?

yes, definitely - this will go next as part of dedicated PR

notandrew · 2018-08-06T21:29:30Z

src/components/Icon/Icon.tsx

    'name',
    'size',
    'xSpacing',
  ]

  static defaultProps = {
-    as: 'i',
-    kind: 'FontAwesome',
+    as: 'span',


Is there a reason you're changing this from the I tag? I is (or was) standard, for the most part.

from MDN

The HTML element represents a range of text that is set off from the normal text for some reason. Some examples include technical terms, foreign language phrases, or fictional character thoughts. It is typically displayed in italic type.

<span> seems to be a more appropriate and neutral element, especially given that we have SVG-based icon as a default option.

notandrew · 2018-08-06T21:36:56Z

src/components/Icon/iconRules.ts

@@ -35,7 +36,35 @@ const getIcon = (kind, name) => {
  return { content, fontFamily }
 }

-const getSize = size => `${sizes.get(size)}em` || '1em'
+const getSize = (size, isFontBased) =>
+  isFontBased ? `${sizes.get(size)}em` : `${sizes.get(size)}rem`


Why ems for fonts and rems for non-fonts?

this is a good observation - it seems that we could use the same principle of relative scaling for both. Will address this

hughreeling · 2018-08-07T08:43:05Z

docs/src/examples/components/Icon/Types/IconExample.shorthand.tsx

@@ -3,20 +3,10 @@ import { Icon } from '@stardust-ui/react'

 const IconExample = () => (
  <div>
-    <Icon size="big" name="chess rock" />


rook - not rock ;)

:) Thank you!

mnajdova · 2018-08-07T12:31:26Z

docs/src/examples/components/Icon/States/IconExampleDisabled.shorthand.tsx

+    <Icon disabled name="umbrella" size="big" />
+    <Icon disabled name="umbrella" size="big" variables={{ color: 'blue' }} />
+    <Icon disabled name="umbrella" size="big" variables={{ color: 'red' }} />
+    <Icon disabled name="umbrella" size="big" variables={{ color: 'orange' }} />


We should update this branch with the latest master and try to fixed the other components that use icons, as well as their examples, because currently those are not working.

mnajdova · 2018-08-07T13:53:45Z

docs/src/examples/components/Icon/Variations/IconExampleBordered.shorthand.tsx

+    <Icon font="FontAwesome" name="calendar alternate outline" bordered />
+    <Icon font="FontAwesome" name="coffee" bordered />
+    <Icon font="FontAwesome" name="compass outline" bordered />
+    <Icon font="FontAwesome" name="area chart" bordered />


Are these still necessary?

should be removed, thanks!

mnajdova

If all other components are consistent with the previous look, I would merge it.

introduce support for SVG icons

fc7e049

kuzhelov requested review from dvdzkwsk, levithomason and mnajdova as code owners August 6, 2018 14:50

changelog

043a3ea

codepretty reviewed Aug 6, 2018

View reviewed changes

notandrew reviewed Aug 6, 2018

View reviewed changes

use relative units everywhere in Icon styles

7756bdc

hughreeling reviewed Aug 7, 2018

View reviewed changes

kuzhelov-ms added 2 commits August 7, 2018 12:10

rock -> rook

763d35b

remove unused import

85f1a57

mnajdova reviewed Aug 7, 2018

View reviewed changes

kuzhelov-ms added 2 commits August 7, 2018 15:11

fix font-based icons

cf8848c

refine condition for rendering SVG icon

ec8ca09

mnajdova reviewed Aug 7, 2018

View reviewed changes

mnajdova approved these changes Aug 7, 2018

View reviewed changes

kuzhelov-ms and others added 3 commits August 7, 2018 22:33

remove unnecessary icon font declarations

0923c71

Merge branch 'master' into feat/add-svg-icons-support

fc6cc90

prettify code

0a7b2b3

kuzhelov merged commit bcd06c7 into master Aug 7, 2018

kuzhelov deleted the feat/add-svg-icons-support branch August 8, 2018 19:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(Icon): add support for SVG #50

feat(Icon): add support for SVG #50

kuzhelov commented Aug 6, 2018 •

edited

codecov bot commented Aug 6, 2018 •

edited

codepretty Aug 6, 2018

kuzhelov Aug 6, 2018

codepretty commented Aug 6, 2018

codepretty Aug 6, 2018

kuzhelov Aug 6, 2018 •

edited

codepretty Aug 6, 2018

kuzhelov Aug 6, 2018

notandrew Aug 6, 2018

kuzhelov Aug 6, 2018

notandrew Aug 6, 2018

kuzhelov Aug 6, 2018

hughreeling Aug 7, 2018

kuzhelov Aug 7, 2018

mnajdova Aug 7, 2018

mnajdova Aug 7, 2018

kuzhelov Aug 7, 2018

mnajdova left a comment

feat(Icon): add support for SVG #50

feat(Icon): add support for SVG #50

Conversation

kuzhelov commented Aug 6, 2018 • edited

Icon

TODO

API Proposal

Name

Font

SVG

codecov bot commented Aug 6, 2018 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codepretty commented Aug 6, 2018

Choose a reason for hiding this comment

kuzhelov Aug 6, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mnajdova left a comment

Choose a reason for hiding this comment

kuzhelov commented Aug 6, 2018 •

edited

codecov bot commented Aug 6, 2018 •

edited

kuzhelov Aug 6, 2018 •

edited