feat(Icon): add props arg to SVG spec#1562
Conversation
kuzhelov
requested review from
dvdzkwsk,
layershifter,
levithomason,
miroslavstastny and
mnajdova
as code owners
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
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. |
| return v[`${size}Size`] | ||
| } | ||
| const getIconSize = (size: SizeValue, v: IconVariables): string => { | ||
| const sizeModifier: IconSizeModifier = v.sizeModifier |
There was a problem hiding this comment.
Can we remove this const now?
| } | ||
|
|
||
| return modifiedSizes[size] && pxToRem(modifiedSizes[size][sizeModifier]) | ||
| return sizeModifier && modifiedSizes[size] && modifiedSizes[size][sizeModifier] |
There was a problem hiding this comment.
Actually, in my opinion the prev conditional is more readable. Is there a solid reason why it was changed?
There was a problem hiding this comment.
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.
| 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) |
3 participants
Fixes #1356 .
This PR provides
propsargument to SVG icon spec. This makes it possible to usepropsvalues, such assize, to define SVG paths accordingly.Example (added
propsarg)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
widthandheight) should be provided.As it is suggested by #1356, in cases if
sizeprop (with, say,smallvalue) is handled by icon's render spec,widthandheightof the icon should remain to be default ones (and shouldn't be changed tosmallvalue). Specifically:However, this behavior may provide unexpected effects to the client for, say, the following code:
Lets suppose that only
callicon provides different SVG path forsmallersize - and, thus, box size of thiscallicon won't be changed tosmallerwidth 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
Iconstyles - 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