feat(icons): add @fluentui/react-icons package #12608
Conversation
|
|
||
| export type SvgIconFuncArg<TProps = ISvgIconProps> = { | ||
| classNames: { [iconSlot: string]: string }; // renamed from classes | ||
| // rtl: boolean; // how do we support this? |
There was a problem hiding this comment.
useRTL hook possibly with an RTL specific context would be ideal. Both v0 and v7 have the rtl flag on the theme object and I don't really see a great way other than React context to get an accurate value unless we just don't support scoped theming.
| const YammerIcon = createSvgIcon({ | ||
| svg: ({ classNames }) => ( | ||
| <svg xmlns="http://www.w3.org/2000/svg" viewBox="0,0,2048,2048"> | ||
| <g className="thing"> |
There was a problem hiding this comment.
className="thing", also classNames doesn't seem to be used.
There was a problem hiding this comment.
I know this className is being used in an example, but this doesn't seem like a good pattern to demonstrate in general. Maybe find another way to re-style the element for the example?
There was a problem hiding this comment.
Ah, I just copied this examples from the Icon.Svg.Example.tsx. Will create new example with showcase of how the things inside the svgs can be styled
There was a problem hiding this comment.
Refactored and improved examples. Please take a look.
|
|
||
| const OneDriveIcon = createSvgIcon({ | ||
| svg: ({ classNames }) => ( | ||
| <svg xmlns="http://www.w3.org/2000/svg" viewBox="0,0,2048,2048"> |
There was a problem hiding this comment.
Should classNames get injected on the root?
There was a problem hiding this comment.
createSvgIcon is doing that. I have two alternatives of the factory once that is for injecting only the svg and another one that allows user to have custom props and define the root element too. Let's see if we will need the both or choose one
packages/office-ui-fabric-react/src/components/Icon/examples/Icon.SvgFactory.Example.tsx
Outdated
Show resolved
Hide resolved
packages/office-ui-fabric-react/src/components/Icon/examples/Icon.SvgFactory.Example.tsx
Outdated
Show resolved
Hide resolved
| const YammerIcon = createSvgIcon({ | ||
| svg: ({ classNames }) => ( | ||
| <svg xmlns="http://www.w3.org/2000/svg" viewBox="0,0,2048,2048"> | ||
| <g className="thing"> |
There was a problem hiding this comment.
I know this className is being used in an example, but this doesn't seem like a good pattern to demonstrate in general. Maybe find another way to re-style the element for the example?
| displayName: 'BorderBlindsIcon', | ||
| }); | ||
|
|
||
| export default BorderBlindsIcon; |
There was a problem hiding this comment.
Have we discussed whether we want to use default or named exports in new code?
There was a problem hiding this comment.
Not really, I just used whatever I was used to :S Let me know what makes more sense and will update it..
There was a problem hiding this comment.
Original thinking with not exporting default was:
A downstream index.ts file might do something like:
export * from './BorderBlindsIcon';Which would not include the default export, but only named ones. That said, we have seen concerns multiple times about export *. And there is NO friction in exporting { default as Foo } explicitly; explicit exports are better anyway in that we don't accidentally leak something unintentionally.
So my 2c is "it's fine".
| { | ||
| ...containerProps, | ||
| ...nativeProps, | ||
| className: css(MS_ICON, classNames.root, className), |
There was a problem hiding this comment.
What's the reasoning for using MS_ICON on the new icons?
There was a problem hiding this comment.
I thought it's nice to have some common classname for all icons. Not sure if this is the right name for it, but we can change it..
There was a problem hiding this comment.
If possible, I'd let the author bring them for the new icons and keep them as vanilla as possible. We made the mistake of using ms-Icon for the existing Icon component and have seen a number of issues where developers do things like:
.ms-Icon { font-size: 22px !important; }We can certainly add them later, but let's try without for now.
Co-Authored-By: David Zearing <[email protected]>
Co-Authored-By: David Zearing <[email protected]>
Co-Authored-By: Elizabeth Craig <[email protected]>
Co-Authored-By: Elizabeth Craig <[email protected]>
|
@ecraig12345 & @dzearing thanks for the early review. Please take another look on the example/usages and the PR description and let's decide on the unknowns so we can faster move the svgs inside the package. |
|
See also PR: #12641 which is adding all svg icons |
mnajdova
changed the title
I would prefer to avoid "withRoot" variant. Is there any reason that the svg can't be considered the root? Is there a benefit of wrapping it in a
I don't think you need to resolve that yet but I think it belongs in a separate context that's easy to reuse without betting on the entire theme. I keep wanting to gravitate to a
Alignment would be awesome, but what specifically? (I don't think this blocks this checkin, but curious specifically which props you want to converge.) |
| const { color1 = 'red', color2 = 'green', color3 = 'blue' } = props; | ||
|
|
||
| // custom styles for the border blinds | ||
| const borderBlindsPart1 = mergeStyles({ |
There was a problem hiding this comment.
I would avoid class names to represent styles which could mutate frequently. Could we use inline styles or css variables?
There was a problem hiding this comment.
(don't let this block checkin, but I think it's important to get the right pattern to replicate.)
There was a problem hiding this comment.
Added inline styles for now. Anyway in the next PR this icon will be remomved, so wouldn't like to spend much time on it..
| import createSvgIcon from '../utils/createSvgIcon'; | ||
|
|
||
| const OneDriveIcon = createSvgIcon({ | ||
| svg: ({ classes }) => ( |
There was a problem hiding this comment.
Minor suggestion/thought:
will there ever be more than 1 class name here? Maybe it should just be className.
There was a problem hiding this comment.
In our experience with react-icons-northstar there are cases like this, so would rather leave it as is at this moment as it is easier for extending this way and won't introduce necessity for changes in all icons in the future..
| const Component: React.FC<React.HTMLAttributes<HTMLSpanElement> & TProps & ISvgIconProps> = props => { | ||
| const { className, style = {} } = props; | ||
|
|
||
| const nativeProps = getNativeProps<React.HTMLAttributes<HTMLElement>>(props, htmlElementProperties); |
There was a problem hiding this comment.
Icons are a hot spot in perf of an app, as they can be rendered tons of times. I wonder if it makes sense to just avoid any unnecessaries altogether:
// get all the things and have ...rest
const { color1, color2, color3, className, ...rest } = props;
return (
<span
role="presentation"
aria-hidden="true"
{...rest}
className={cx(classes, className)}
/>);There was a problem hiding this comment.
Yes but that would mean that we may end up with wrong props on the root, which is not the case with any other component in both libraries... Would rather merge it as is now and we can measure perf and decide in the next phase...
# Conflicts: # packages/office-ui-fabric-react/package.json # yarn.lock
|
@ecraig12345 can you double check the dependencies version. I used |
Asset size changesSize Auditor did not detect a change in bundle size for any component! Baseline commit: d4e4953524017e8e8238cc245eaa83e91dec66c9 (build) |
Perf AnalysisNo significant results to display. All results
Perf Analysis (Fluent)Perf comparison
Perf tests with no regressions
|
|
🎉 Handy links: |
* wip * -added example svg icons * -added props support by diff factory * Update packages/react-icons/src/components/BorderBlindsIcon.tsx Co-Authored-By: David Zearing <[email protected]> * Update packages/react-icons/src/components/BorderBlindsIcon.tsx Co-Authored-By: David Zearing <[email protected]> * Update packages/react-icons/src/utils/createSvgIcon.ts Co-Authored-By: Elizabeth Craig <[email protected]> * Update packages/react-icons/README.md Co-Authored-By: Elizabeth Craig <[email protected]> * -cleanups -removed circular dependencies * -removed comments * -improvements * -exported utilities * -updated readme * -addressed comments * -added tests * -added dependencies * -removed merge conflicts changes * -picked changes from master * -reverted changes * -updated package json * Change files * Adding snapshot. Co-authored-by: David Zearing <[email protected]> Co-authored-by: Elizabeth Craig <[email protected]>
This PR is adding new
@fluentui/react-iconspackage that will contain utilities for creating icons, as well as set of svg icons.There are currently two proposals on the svg factory function:
createSvgIconthat can be used in the following manner:The root
spanelement with all processed props is created automatically here inside thecreateSvgIconfactory.One we decide on the factory used, I will migrate all svg icons.
NEEDS SOME HELP/SUGGESTIONS
Things that we need to decide on:
createSvgIconSvgIconPropson v0 and v7, or should we keep the API simple for the v7? - This will help when we want to merge the icons set - will be aligned as next step