Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat(icons): add factory for creating svg icons #12319

Merged
merged 41 commits into from
Mar 20, 2020
Merged

feat(icons): add factory for creating svg icons #12319

merged 41 commits into from
Mar 20, 2020

Conversation

mnajdova
Copy link
Contributor

@mnajdova mnajdova commented Mar 16, 2020 •
edited

This PR introduces SvgIcon utility component that can be used for creating SVG icons. The motitivation of this is mostly for reducing the bundle size. Currently all icons that we have (all svgs) are part of the gigantic theme object. With this factory, we are allowing the users to create themable component icons, without including the svgs inside the theme object.

For this to work there are two ways of creating the icons.

  1. Using the factory for generating svg icons:
const teamsIconClassNames = {
  filled: 'ui-icon__filled',
  outline: 'ui-icon__outline'
};

const callVideoSvg = ({ classes }) => (
  <svg className={classes.svg} viewBox="8 8 16 16" role="presentation" focusable="false">
    <g className={cx(teamsIconClassNames.outline, classes.outlinePart)}>
      <path d="..." />
      <path d="..." />
    </g>
    <g className={cx(teamsIconClassNames.filled, classes.filledPart)}>
      <path d="..." />
      <path d="..." />
    </g>
  </svg>
);

const CallVideoIcon = createSvgIcon({ svg: callVideoSvg, displayName: 'CallVideoIcon'});
  1. Creating simple component that will use the SvgIcon under the hood (this will result with two components inside the React tree)
const BookIcon = props => (
  <SvgIcon {...props}>
    {({ classes }) => (
      <svg role="presentation" focusable="false" viewBox="8 8 16 16" className={classes.svg}>
        <path d="..." />
      </svg>
    )}
  </SvgIcon>
);

@msft-github-bot msft-github-bot added the Fluent UI react-northstar (v0) Work related to Fluent UI V0 label Mar 16, 2020
mnajdova
mnajdova commented Mar 16, 2020
View reviewed changes
packages/fluentui/react/src/themes/teams/components/Icon/svgIconStyles.ts Outdated
}
};

const svgIconStyles: ComponentSlotStylesPrepared<SvgIconStylesProps, IconVariables> = {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Simplified iconStyles that do not consider font icons and colors

@msft-github-bot
Copy link
Contributor

msft-github-bot commented Mar 16, 2020 •
edited

Perf Analysis

No significant results to display.

All results

Scenario Master Ticks PR Ticks Status
BaseButton 774 752
BaseButton (experiments) 884 883
DefaultButton 1001 979
DefaultButton (experiments) 1722 1747
DetailsRow 3182 3142
DetailsRow (fast icons) 3291 3237
DetailsRow without styles 2985 2976
DocumentCardTitle with truncation 1557 1588
MenuButton 1345 1302
MenuButton (experiments) 3263 3281
PrimaryButton 1113 1107
PrimaryButton (experiments) 1851 1857
SplitButton 2887 2898
SplitButton (experiments) 6632 6629
Stack 425 424
Stack with Intrinsic children 1011 1017
Stack with Text children 3728 3744
Text 337 341
Toggle 803 797
Toggle (experiments) 2074 2064
button 62 62

Perf Analysis (Fluent)

Perf comparison
Status Scenario Fluent TPI Fabric TPI Ratio Iterations Ticks
🔧 Avatar.Fluent 0.54 0.46 1.17:1 2000 1087
🦄 Button.Fluent 0.09 0.17 0.53:1 5000 466
🔧 Checkbox.Fluent 0.74 0.38 1.95:1 1000 741
🔧 Dialog.Fluent 0.39 0.19 2.05:1 5000 1956
🔧 Dropdown.Fluent 3.67 0.46 7.98:1 1000 3667
🔧 Icon.Fluent 0.17 0.04 4.25:1 5000 839
🎯 Image.Fluent 0.07 0.1 0.7:1 5000 326
🔧 Slider.Fluent 1.6 0.39 4.1:1 1000 1602
🔧 Text.Fluent 0.07 0.02 3.5:1 5000 367
🦄 Tooltip.Fluent 0.12 15.37 0.01:1 5000 598

🔧 Needs work     🎯 On target     🦄 Amazing

Perf tests with no regressions
Scenario Current PR Ticks Baseline Ticks Ratio
IconMinimalPerf.default 428 382 1.12:1
Button.Fluent 466 437 1.07:1
AnimationMinimalPerf.default 648 624 1.04:1
ChatMinimalPerf.default 575 552 1.04:1
ChatWithPopoverPerf.default 638 616 1.04:1
DividerMinimalPerf.default 1022 980 1.04:1
HeaderMinimalPerf.default 558 539 1.04:1
InputMinimalPerf.default 1110 1072 1.04:1
ListCommonPerf.default 1050 1007 1.04:1
PortalMinimalPerf.default 305 293 1.04:1
Checkbox.Fluent 741 710 1.04:1
BoxMinimalPerf.default 351 342 1.03:1
ChatDuplicateMessagesPerf.default 433 419 1.03:1
GridMinimalPerf.default 864 842 1.03:1
HierarchicalTreeMinimalPerf.default 1017 990 1.03:1
TextAreaMinimalPerf.default 3311 3207 1.03:1
ButtonSlotsPerf.default 631 616 1.02:1
DialogMinimalPerf.default 1983 1946 1.02:1
FlexMinimalPerf.default 269 263 1.02:1
ProviderMergeThemesPerf.default 1419 1387 1.02:1
SegmentMinimalPerf.default 1122 1097 1.02:1
SliderMinimalPerf.default 1629 1602 1.02:1
Avatar.Fluent 1087 1069 1.02:1
AccordionMinimalPerf.default 229 226 1.01:1
ListNestedPerf.default 950 939 1.01:1
LoaderMinimalPerf.default 1108 1097 1.01:1
ProviderMinimalPerf.default 663 658 1.01:1
ReactionMinimalPerf.default 2534 2504 1.01:1
TableMinimalPerf.default 657 650 1.01:1
VideoMinimalPerf.default 867 858 1.01:1
AlertMinimalPerf.default 631 630 1:1
CheckboxMinimalPerf.default 3295 3296 1:1
FormMinimalPerf.default 930 930 1:1
HeaderSlotsPerf.default 1687 1685 1:1
MenuButtonMinimalPerf.default 1584 1577 1:1
PopupMinimalPerf.default 253 253 1:1
RadioGroupMinimalPerf.default 539 540 1:1
SplitButtonMinimalPerf.default 12656 12714 1:1
TreeMinimalPerf.default 1182 1181 1:1
Dialog.Fluent 1956 1952 1:1
Dropdown.Fluent 3667 3657 1:1
Text.Fluent 367 368 1:1
Tooltip.Fluent 598 596 1:1
AttachmentSlotsPerf.default 3723 3746 0.99:1
CarouselMinimalPerf.default 2075 2089 0.99:1
DropdownManyItemsPerf.default 1481 1500 0.99:1
DropdownMinimalPerf.default 3632 3683 0.99:1
EmbedMinimalPerf.default 5598 5632 0.99:1
LayoutMinimalPerf.default 657 664 0.99:1
ListMinimalPerf.default 432 438 0.99:1
ListWith60ListItems.default 1183 1200 0.99:1
StatusMinimalPerf.default 593 602 0.99:1
CustomToolbarPrototype.default 3761 3787 0.99:1
TooltipMinimalPerf.default 852 864 0.99:1
Icon.Fluent 839 850 0.99:1
AvatarMinimalPerf.default 562 574 0.98:1
ImageMinimalPerf.default 325 330 0.98:1
ItemLayoutMinimalPerf.default 2066 2111 0.98:1
LabelMinimalPerf.default 368 377 0.98:1
MenuMinimalPerf.default 2026 2061 0.98:1
AttachmentMinimalPerf.default 940 966 0.97:1
TextMinimalPerf.default 360 372 0.97:1
ToolbarMinimalPerf.default 1093 1131 0.97:1
TreeWith60ListItems.default 221 227 0.97:1
Image.Fluent 326 336 0.97:1
RefMinimalPerf.default 199 207 0.96:1
Slider.Fluent 1602 1668 0.96:1
ButtonMinimalPerf.default 144 154 0.94:1

@size-auditor
Copy link

size-auditor bot commented Mar 16, 2020 •
edited

Asset size changes

Size Auditor did not detect a change in bundle size for any component!

Baseline commit: 898df7c6729e223b458fa5ab80813f6cad7da240 (build)

layershifter
layershifter reviewed Mar 17, 2020
View reviewed changes
packages/fluentui/react/src/components/Icon/SvgIcon.tsx Outdated
size,
xSpacing
}),
mapPropsToInlineStyles: () => ({ className, design, styles, variables }),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
mapPropsToInlineStyles: () => ({ className, design, styles, variables }),
mapPropsToInlineStyles: () => ({ className }),

For now, let's keep compatibility with other components, but I think that these props (styles, variables) should be removed

@layershifter
Copy link
Member

  1. Do we have a migration plan? Codemods? codemod or other?
  2. Are we going to create a separate package for icons? Will we move factory to react-bindings?
  3. What our strategy for components? Something like below?
<MenuItem icon={{ content: <BookIcon /> }} />
<MenuItem icon={<BookIcon />} />

Will icon be a Box shorthand?

layershifter
layershifter reviewed Mar 20, 2020
View reviewed changes
packages/fluentui/react-bindings/src/icons/createSvgIcon.tsx Outdated
export const SvgIconClassName = 'ui-icon';
export const SvgIconDisplayName = 'SvgIcon';

export const SvgIconHandledProps: (keyof SvgIconProps)[] = [
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
export const SvgIconHandledProps: (keyof SvgIconProps)[] = [
const svgIconHandledProps: (keyof SvgIconProps)[] = [

I don't think that we need to export this

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The same is used in the SvgIcon component, that's why we need to export it

@mnajdova @layershifter
Update packages/fluentui/react-bindings/test/icons/createSvgIcon-test…
baeab69 
….tsx

Co-Authored-By: Oleksandr Fediashov <alexander.mcgarret@gmail.com>
@mnajdova mnajdova merged commit 13e558c into microsoft:master Mar 20, 2020
@mnajdova mnajdova mentioned this pull request Mar 23, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@layershifter layershifter layershifter approved these changes

@miroslavstastny miroslavstastny miroslavstastny approved these changes

@dzearing dzearing Awaiting requested review from dzearing

@ecraig12345 ecraig12345 Awaiting requested review from ecraig12345

@JasonGore JasonGore Awaiting requested review from JasonGore

@jdhuntington jdhuntington Awaiting requested review from jdhuntington

@jurokapsiar jurokapsiar Awaiting requested review from jurokapsiar

@levithomason levithomason Awaiting requested review from levithomason

@pompomon pompomon Awaiting requested review from pompomon

@silviuaavram silviuaavram Awaiting requested review from silviuaavram

Assignees

@aneeshack4 aneeshack4

Labels
Fluent UI react-northstar (v0) Work related to Fluent UI V0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@mnajdova @msft-github-bot @layershifter @miroslavstastny @silviuaavram @aneeshack4