feat(Icon): add props arg to SVG spec #1562

kuzhelov · 2019-07-01T09:46:35Z

Fixes #1356 .

This PR provides props argument to SVG icon spec. This makes it possible to use props values, such as size, to define SVG paths accordingly.

Example (added `props` arg)

// call.tsx
export default {
  icon: ({ classes, props }) => (
     {props.size === 'small' && (
           <svg>
            ...
          </svg>
     )}
     ...
} as TeamsSvgIconSpec

Opened question on icon's box sizes (width and height)

It still remains to be an opened question whether changes to logic of applying box sizes to SVG element (such as width and height) should be provided.

As it is suggested by #1356, in cases if size prop (with, say, small value) is handled by icon's render spec, width and height of the icon should remain to be default ones (and shouldn't be changed to small value). Specifically:

but if an icon has a smaller version (either new SVG or paths or ??) then it should use that. If not, then it should scale via css.

However, this behavior may provide unexpected effects to the client for, say, the following code:

// this icon renders different SVG path for 'smaller', 
// but its box's width and height doesn't change with 'size' prop
<Icon name='call' size='smaller' bordered> 

<Icon name='video' size='smaller' bordered>

Lets suppose that only call icon provides different SVG path for smaller size - and, thus, box size of this call icon won't be changed to smaller width and height values. In that case this will be the rendered result:

Even given that both icons were rendered as smaller, they have different box sizes which, in its turn, breaks alignment.

My proposal here would be to guarantee that box sizing algorithms work the same way for icons of any type, the same invariant we had before for Icon styles - as this guarantees proper alignment for any icons defined. Should note that this strategy will be sufficient enough to address the case of #1359.

However, it may be the case that I am missing some use cases missing with that - so, lets discuss and get to decision before merging the PR. FYI @codepretty

codecov · 2019-07-01T10:01:22Z

Codecov Report

Merging #1562 into master will decrease coverage by 0.01%.
The diff coverage is 28.57%.

@@            Coverage Diff             @@
##           master    #1562      +/-   ##
==========================================
- Coverage   71.55%   71.54%   -0.02%     
==========================================
  Files         838      838              
  Lines        6858     6859       +1     
  Branches     1947     1968      +21     
==========================================
  Hits         4907     4907              
- Misses       1945     1946       +1     
  Partials        6        6

Impacted Files	Coverage Δ
.../src/themes/teams/components/Icon/iconVariables.ts	`50% <ø> (ø)`	⬆️
packages/react/src/components/Icon/Icon.tsx	`100% <100%> (ø)`	⬆️
...act/src/themes/teams/components/Icon/iconStyles.ts	`14.28% <16.66%> (-0.43%)`	⬇️

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 ca579ed...85fa60d. Read the comment docs.

codecov · 2019-07-01T10:01:22Z

Codecov Report

Merging #1562 into master will not change coverage.
The diff coverage is 33.33%.

@@           Coverage Diff           @@
##           master    #1562   +/-   ##
=======================================
  Coverage   71.67%   71.67%           
=======================================
  Files         848      848           
  Lines        6916     6916           
  Branches     1964     1965    +1     
=======================================
  Hits         4957     4957           
  Misses       1953     1953           
  Partials        6        6

Impacted Files	Coverage Δ
.../src/themes/teams/components/Icon/iconVariables.ts	`50% <ø> (ø)`	⬆️
packages/react/src/components/Icon/Icon.tsx	`100% <100%> (ø)`	⬆️
...act/src/themes/teams/components/Icon/iconStyles.ts	`14.7% <20%> (ø)`	⬆️

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 442ff22...e1b6a39. Read the comment docs.

mnajdova · 2019-07-02T11:39:42Z

My proposal here would be to guarantee that box sizing algorithms work the same way for icons of any type, the same invariant we had before for Icon styles - as this guarantees proper alignment for any icons defined. Should note that this strategy will be sufficient enough to address the case of #1359.

I agree with this, all types of icons should behave the same, and depending on the theme's requirement, we can make the box (border around the icon) to grow or always be the same for all icons. For the icons that do not support different sizes, the box will still always be the same, as well as the icon size.

mnajdova · 2019-07-03T14:21:46Z

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

-    return v[`${size}Size`]
-  }
+const getIconSize = (size: SizeValue, v: IconVariables): string => {
+  const sizeModifier: IconSizeModifier = v.sizeModifier


Can we remove this const now?

mnajdova · 2019-07-03T14:23:45Z

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

@@ -35,7 +33,9 @@ const getIconSize = (size: SizeValue, sizeModifier: IconSizeModifier, v: IconVar
    },
  }

-  return modifiedSizes[size] && pxToRem(modifiedSizes[size][sizeModifier])
+  return sizeModifier && modifiedSizes[size] && modifiedSizes[size][sizeModifier]


Actually, in my opinion the prev conditional is more readable. Is there a solid reason why it was changed?

this change was necessary to remove branching at method's start:

if (!sizeModifier) { return v[`${size}Size`] }

Essentially, in this method we have two branches to handle, thus it is an expected thing that we need just one if for that. Previously there were two ifs (at start of the method and at return) for, essentially, differentiate between the same two branches (sizeModifier is provided or not), and that has complicated the overall's function logic - and now there is only one.

mnajdova · 2019-07-03T14:23:58Z

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

@@ -80,7 +80,7 @@ const iconStyles: ComponentSlotStylesInput<IconProps, IconVariables> = {

  svg: ({ props: { size, color, disabled, rotate }, variables: v }): ICSSInJSStyle => {
    const colors = v.colorScheme[color]
-    const iconSizeInRems = getIconSize(size, v.sizeModifier, v)
+    const iconSizeInRems = getIconSize(size, v)


add props arg to svg icon spec

9d4ee3d

kuzhelov requested review from dvdzkwsk, layershifter, levithomason, miroslavstastny and mnajdova as code owners July 1, 2019 09:46

kuzhelov added the 🚧 WIP label Jul 1, 2019

leave size modifier to be an optional var

85fa60d

kuzhelov added 🚀 ready for review and removed 🚧 WIP labels Jul 1, 2019

mnajdova reviewed Jul 3, 2019

View reviewed changes

remove sizeModifier const

3151a79

mnajdova approved these changes Jul 3, 2019

View reviewed changes

kuzhelov and others added 4 commits July 3, 2019 17:40

Merge branch 'master' into feat/add-props-to-icon-spec

1e40659

upd changelog

0a301f4

Merge branch 'master' into feat/add-props-to-icon-spec

3aaa319

upd changelog

e1b6a39

kuzhelov merged commit 8c9ef6b into master Jul 8, 2019

delete-merged-branch bot deleted the feat/add-props-to-icon-spec branch July 8, 2019 03:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(Icon): add props arg to SVG spec #1562

feat(Icon): add props arg to SVG spec #1562

kuzhelov commented Jul 1, 2019 •

edited

Loading

codecov bot commented Jul 1, 2019 •

edited

Loading

codecov bot commented Jul 1, 2019 •

edited

Loading

mnajdova commented Jul 2, 2019

mnajdova Jul 3, 2019

mnajdova Jul 3, 2019

kuzhelov Jul 3, 2019

mnajdova Jul 3, 2019

feat(Icon): add props arg to SVG spec #1562

feat(Icon): add props arg to SVG spec #1562

Conversation

kuzhelov commented Jul 1, 2019 • edited Loading

Example (added props arg)

Opened question on icon's box sizes (width and height)

codecov bot commented Jul 1, 2019 • edited Loading

Codecov Report

codecov bot commented Jul 1, 2019 • edited Loading

Codecov Report

mnajdova commented Jul 2, 2019

mnajdova Jul 3, 2019

Choose a reason for hiding this comment

mnajdova Jul 3, 2019

Choose a reason for hiding this comment

kuzhelov Jul 3, 2019

Choose a reason for hiding this comment

mnajdova Jul 3, 2019

Choose a reason for hiding this comment

kuzhelov commented Jul 1, 2019 •

edited

Loading

Example (added `props` arg)

codecov bot commented Jul 1, 2019 •

edited

Loading

codecov bot commented Jul 1, 2019 •

edited

Loading