feat(icons): add support for styling SVG icons #183

kuzhelov · 2018-09-04T21:34:12Z

TODO

Update CHANGELOG.md

Problem

Introduce mechanisms in Stardust to provide/extend set of SVG icons by themes.

Further problem analysis

Essentially, task of introducing custom SVG icons (by means of themes) could be reduced to solving the following tasks

add mechanisms for rendering custom SVG node
add support for styling different parts of rendered SVG

Functionality that aims to solve these tasks should be added as part of core Stardust functionality, and there should be no assumptions made on Stardust's side about specific structure of rendered SVGs or their styling aspects - suggested mechanisms should be flexible enough to meet different needs of consumers (theme authors).

Proposed solution

allow each theme to provide collection of named functions: where name corresponds to the name of the icon, and function's responsibility is to provide SVG node, with styles (classes) being provided as function's argument
conventions about both styling aspects, as well as structure of SVG for each individual icon are entirely theme's concerns
changes in core Stardust interface
- extend theme properties to include svgIcons object - this maps icon's name to the icon rendering function: (iconClasses) => SVG as ReactNode. Please, note that although iconClasses is something that is related to Icon component, this is an aspect that is entirely controlled by theme - thus, all the styling aspects could be customized by theme

Use case

Suppose that there is a task for theme developer to introduce custom set of SVG icons. Stardust requires them to be exported in the following form from theme:

const icons = {
   'iconName': (classes) => (<svg>...</svg>)
   ....
}

export default theme = {
   ... (componentStyles, componentVariables, fonts, etc) ...
  svgIcons: icons
}

With that the question of 'what' should be rendered (for each particular icon) is solved - the only question remains is how it should be styled. For that classes object is provided as argument, so that those could be used to style individual parts of the icon's element tree - and theme developer has full control over introducing necessary set of parts and styles for Icon:

// iconStyles.js file
export default {
  root: ...
  ...
  filledSvg: ({ props, variables }) => ({
    fill: variables.filledIconColor,
    ...
  }),
  ...
}

// svgIcons.js file
const icons = {
   'filled': (classes) => (<svg className={classes.filledSvg}>...</svg>)
   ....
}

codecov · 2018-09-04T21:37:32Z

Codecov Report

Merging #183 into master will increase coverage by 0.25%.
The diff coverage is 77.41%.

@@            Coverage Diff             @@
##           master     #183      +/-   ##
==========================================
+ Coverage   67.82%   68.08%   +0.25%     
==========================================
  Files         101      113      +12     
  Lines        1386     1438      +52     
  Branches      265      268       +3     
==========================================
+ Hits          940      979      +39     
- Misses        443      456      +13     
  Partials        3        3

Impacted Files	Coverage Δ
src/themes/teams/components/Icon/iconStyles.ts	`25% <100%> (+2.08%)`	⬆️
...mes/teams/components/Icon/svg/icons/teamCreate.tsx	`100% <100%> (ø)`
...rc/themes/teams/components/Icon/svg/icons/more.tsx	`100% <100%> (ø)`
...components/Icon/svg/icons/callIncomingVideoOff.tsx	`100% <100%> (ø)`
...teams/components/Icon/svg/icons/callMicrophone.tsx	`100% <100%> (ø)`
...ms/components/Icon/svg/icons/callIncomingVideo.tsx	`100% <100%> (ø)`
src/components/Icon/Icon.tsx	`100% <100%> (ø)`	⬆️
src/themes/teams/index.ts	`100% <100%> (ø)`	⬆️
...ts/Icon/svg/icons/callControlStopPresentingNew.tsx	`100% <100%> (ø)`
...ms/components/Icon/svg/icons/callMicrophoneOff.tsx	`100% <100%> (ø)`
... and 18 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 e56b62c...0db43ad. Read the comment docs.

kuzhelov · 2018-09-04T23:55:27Z

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

-  svg: ({ variables: v }): ICSSInJSStyle => ({
-    fill: v.color,
-  }),
+  svg: getSvgStyle('svg'),


yes-yes, let me guess what you might think about :) Actually, this is rather a 'have to' thing, because currently we don't have any possibility to introduce variable set of style parts, such that, for example, if props.name is 'endCall' (a name of icon), there would be only parts in styles that are relevant to that icon (root, phone, cross-line, etc). However, there are couple of things we should consider before, potentially, making this move:

if set of styling parts will be decided by values from props, it will drastically complicate overall picture - one thing that immediately comes to my mind is increased debugging complexity

problem of hardcoded style parts - the ones that should fit all component from provided set, actually, is not that serious: generally there are just handful different parts of the icon that should be styled differently at any given moment, and these parts are entirely theme's aspects: there are no restrictions introduced from core components' functionality on how these style parts should be named, organised or consumed

With that being said, I rather see suggested approach as being flexible enough to meet quite broad set of introducing styles for custom set of icons.

Should we add at least first add as many paths properties here as there is in our svg icons so far, and add increase the number of them accordingly if we are adding icons with multiple path parts? Maybe we can generate the paths properties in the begging of the file in order to avoid them being added like this?

Maybe we can generate the paths properties in the begging of the file in order to avoid them being added like this?

yes, but this leads to exact problem I've described above - where the solution is to somehow allow variable amount of component parts being declared for styles (ideally, this number should be dependent on props). While it is an option, there are some drawbacks associated with it - have mentioned them above.

Should we add at least first as many paths properties here as there is in our svg icons so far, and add increase the number of them accordingly if we are adding icons with multiple path parts

we may consider to do this, yes

If we have just as many paths properties as our svg icons actually have, I am okay with them being hardcoded, I see your point. With this, we will be aware about what each of those represent.

mnajdova · 2018-09-05T12:22:48Z

types/theme.d.ts

+// SVG Icons
+// ========================================================
+
+type Classes = { [iconPart: string]: string }


This seems to be like a general type that can be reused in some other places. Should we change the definition with something like type Classes = { [componentPart: string]: string } and place it in the utils section? If not, maybe we should change the name to IconClasses in order to be more specific.

yes, thanks - let me make this move if we'll agree on the general approach

Note, this type exists in this file already:

https://github.com/stardust-ui/react/blob/e460060d318cbe3018a48f4625dfc9b263625b12/types/theme.d.ts#L107

mnajdova · 2018-09-05T12:24:25Z

src/themes/teams/index.ts

+import { default as svgIconsAndStyles } from './components/Icon/svg'
+import { IconSpec } from './components/Icon/svg/types'
+
+const svgIcons = Object.keys(svgIconsAndStyles as { [iconName: string]: IconSpec }).reduce<


Is this worth converting to a type: { [iconName: string]: IconSpec }

mnajdova · 2018-09-05T12:33:27Z

src/themes/teams/components/Icon/svg/icons/call.tsx

+
+export default {
+  icon: classes => (
+    <svg viewBox="0 0 32 32" role="presentation" focusable="false" className={classes.svg}>


Should we pass the classes to the svg element here?

yes - but this is intentionally left to icons' maintainers, as current system (where both CSS class selectors and DOM selectors are used for styling) should be refactored

mnajdova · 2018-09-05T12:34:11Z

src/themes/teams/components/Icon/svg/icons/callControlPresentNew.tsx

+export default (classes => (
+  <svg viewBox="0 0 32 32" role="presentation">
+    <path
+      d="M22.5,9h-13C8.672852,9,8,9.672852,8,10.5v9C8,20.327148,8.672852,21,9.5,21H13c0.276367,0,0.5-0.223633,0.5-0.5


Should we pass the classes to the svg element and the paths here? Is this where the path1, path2 etc. objects from the iconStyles will be used?

Is this where the path1, path2 etc. objects from the iconStyles will be used?

yes, exactly. I would leave this work to icons' maintainers, as well as making final decision on how styling parts should be named for icons. Once we'll agree that provided mechanisms are sufficient enough to meet their needs, the task will reduce only to styling aspects that I would rather prefer to be addressed by design folks

mnajdova · 2018-09-05T12:38:19Z

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

-  svg: ({ variables: v }): ICSSInJSStyle => ({
-    fill: v.color,
-  }),
+  svg: getSvgStyle('svg'),


Should we add at least first add as many paths properties here as there is in our svg icons so far, and add increase the number of them accordingly if we are adding icons with multiple path parts? Maybe we can generate the paths properties in the begging of the file in order to avoid them being added like this?

jurokapsiar · 2018-09-06T12:47:46Z

Heads up for the future: For RTL we will need to be able to flip certain icons, for example pointing arrows. It should be possible to do this in a generic way (applying css transformations on the svg). It is important that approach proposed in this PR will allow us to do that in the future.

kuzhelov · 2018-09-06T20:24:59Z

@jurokapsiar, this is a very good point. With provided functionality we have couple of ways to address that, essentially

we could address it on the core level, by introducing general 'RTL transform' functionality to Icon component. This approach will mimic the one that is currently used in Teams, where class name is added to root SVG element to make this transform
or we could pass rtl value to SVG function (so it will be (classes, rtl) => React.ReactNode). Although this approach ptovides more control over rendered SVG tree, I think that at early stages we should limit amount of data passed to icon's rendering function, so that overall picture will be simplier

miroslavstastny · 2018-09-07T08:15:42Z

src/lib/mergeThemes.ts

@@ -159,6 +160,9 @@ const mergeThemes = (...themes: IThemeInput[]): IThemePrepared => {

    acc.componentStyles = mergeThemeStyles(acc.componentStyles, next.componentStyles)

+    // Merge icons set, last one wins in case of collisions
+    acc.svgIcons = { ...acc.svgIcons, ...(next.svgIcons || {}) }


sure, will introduce them once we will agree on the approach

miroslavstastny · 2018-09-07T09:05:19Z

src/themes/teams/components/Icon/svg/icons/index.ts

+export { default as call } from './call'
+export { default as callEnd } from './callEnd'
+
+export { default as callVideo } from './callIncomingVideo'


What's the reason for exporting with a name different from filename?

the reason was to save mapping from the icon that is provided (with simple name) to the one that was originally used by Teams (longer name) - with that I thought it will be easier to track which one corresponds to which at the initial stages of adoption. Absolutely ok to make any renaming that will suit Teams theme devs

kuzhelov · 2018-09-20T23:56:29Z

closing as this set of changes will be introduced by means of #260

add SVG icons to teams theme

ef47940

kuzhelov requested review from dvdzkwsk, levithomason, miroslavstastny and mnajdova as code owners September 4, 2018 21:34

kuzhelov commented Sep 4, 2018

View reviewed changes

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

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

0db43ad

mnajdova reviewed Sep 5, 2018

View reviewed changes

miroslavstastny reviewed Sep 7, 2018

View reviewed changes

udpate theme icons api, add example enumerating icons

3fd1512

levithomason force-pushed the feat/add-svg-icons branch from 45c6f35 to 3fd1512 Compare September 13, 2018 19:40

levithomason mentioned this pull request Sep 14, 2018

feat: add Attachment component #220

Merged

9 tasks

kuzhelov mentioned this pull request Sep 14, 2018

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

Closed

kuzhelov added ready for merge postponed Item is postponed and will be reconsidered in future and removed 🚀 ready for review labels Sep 14, 2018

kuzhelov mentioned this pull request Sep 20, 2018

feat: themable icons #260

Merged

1 task

kuzhelov closed this Sep 20, 2018

levithomason deleted the feat/add-svg-icons branch October 11, 2018 21:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(icons): add support for styling SVG icons #183

feat(icons): add support for styling SVG icons #183

kuzhelov commented Sep 4, 2018 •

edited

Loading

codecov bot commented Sep 4, 2018 •

edited

Loading

kuzhelov Sep 4, 2018 •

edited

Loading

mnajdova Sep 5, 2018 •

edited

Loading

kuzhelov Sep 5, 2018

mnajdova Sep 5, 2018

mnajdova Sep 5, 2018

kuzhelov Sep 5, 2018

levithomason Sep 13, 2018

mnajdova Sep 5, 2018

mnajdova Sep 5, 2018

kuzhelov Sep 5, 2018

mnajdova Sep 5, 2018

kuzhelov Sep 5, 2018

mnajdova Sep 5, 2018 •

edited

Loading

jurokapsiar commented Sep 6, 2018

kuzhelov commented Sep 6, 2018

miroslavstastny Sep 7, 2018

kuzhelov Sep 7, 2018

miroslavstastny Sep 7, 2018

kuzhelov Sep 7, 2018

kuzhelov commented Sep 20, 2018

feat(icons): add support for styling SVG icons #183

feat(icons): add support for styling SVG icons #183

Conversation

kuzhelov commented Sep 4, 2018 • edited Loading

TODO

Problem

Further problem analysis

Proposed solution

Use case

codecov bot commented Sep 4, 2018 • edited Loading

Codecov Report

kuzhelov Sep 4, 2018 • edited Loading

Choose a reason for hiding this comment

mnajdova Sep 5, 2018 • edited Loading

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 Sep 5, 2018 • edited Loading

Choose a reason for hiding this comment

jurokapsiar commented Sep 6, 2018

kuzhelov commented Sep 6, 2018

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

kuzhelov commented Sep 20, 2018

kuzhelov commented Sep 4, 2018 •

edited

Loading

codecov bot commented Sep 4, 2018 •

edited

Loading

kuzhelov Sep 4, 2018 •

edited

Loading

mnajdova Sep 5, 2018 •

edited

Loading

mnajdova Sep 5, 2018 •

edited

Loading