feat(Icon): Redlining icons #441
Conversation
codepretty
requested review from
dvdzkwsk,
kuzhelov,
levithomason,
miroslavstastny and
mnajdova
as code owners
codepretty
changed the title
Codecov Report
@@ Coverage Diff @@
## master #441 +/- ##
======================================
Coverage 89.2% 89.2%
======================================
Files 41 41
Lines 1417 1417
Branches 202 177 -25
======================================
Hits 1264 1264
Misses 149 149
Partials 4 4
Continue to review full report at Codecov.
|
| <Label content="Default" /> | ||
| </div> | ||
| <div style={{ marginBottom: '1.2rem' }}> | ||
| <Label content="Before" /> | ||
| <Icon name="umbrella" xSpacing="before" size="big" /> |
docs/src/examples/components/Popup/Types/PopupCustomTargetExample.shorthand.tsx
Outdated
Show resolved
Hide resolved
docs/src/examples/components/Icon/Usage/IconSetExample.shorthand.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
do like the approach taken - have left several minor comments, please, feel free to decide on whether they should be addressed 👍Please, just make CHANGELOG entry before merging these changes.
Also have made a not to myself in regards to styles prop of the icon with empty object value.
| } | ||
| }, | ||
| }, | ||
| styles: {}, |
There was a problem hiding this comment.
note to myself - if there are no icon-specific styles (like the ones that will be applied to call icon exclusively), this styles prop should be safe to delete from this object
The Icon examples are indeed to tied to Teams theme now, but see this as a normal thing for now at least, and speaking of the last section on the Icon's docs page (where all the icons are shown) - this close tie is inevitable there. In either case, it seems that we are in a pretty good position to address this "close tie" problem for icons if necessary - as these changes don't make the problem any worse. Speaking of the behavior in case if icon is absent in theme's collection - I would expect that there will be a warning/error message logged to the dev console, so this fact won't be unnoticed; speaking of rendering process - yes, absolutely agree that this fact shouldn't result in exceptions being thrown: whole components' tree should be just rendered without this missing icon. |
|
@codepretty There are a bunch of additional icons that we would need support for. Would they be added as part of this PR? Icons needed
|
|
@kohlikohl importing icons is a separate PR. It is almost ready, just want to get this checked in first. |
Icon style updates for Teams theme
The style updates for Teams icons is mostly around the size of the icons. The normal (default) Icon size matches the Teams spec of being a 16px x 16px space. Other size variations of Icons will be addressed in a future PR.
Before
After
This shows the size update for both SVGs and Icon fonts.
Filled and Outline versions of Teams Icon
Part of this PR includes updates to show either a filled or outline version of the icon.
API update
Changing viewBox for Teams SVGs
We're changing the viewBox of all (or most?) Teams icons. Default icons are built with built-in padding, a viewBox of 32x32px and a 16x16px icon size. We changed the viewBox to remove the bounding box and just have the icon, which remains the same 16x16px size.
The reason for this is because if you want an icon to appear to not have any space you would need to do some negative margin hacks.
Before
Now
Question: Are the icons examples too tied to Teams theme? In a default theme, what would the expectation be if there is no icon provider shown? Today, it fails silently by not showing anything which feels right to me, but wanted to get others opinions..