Skip to content

Update Avatar to use Fluent icons#18566

Merged
behowell merged 17 commits into
microsoft:masterfrom
behowell:avatar-icon
Aug 4, 2021
Merged

Update Avatar to use Fluent icons#18566
behowell merged 17 commits into
microsoft:masterfrom
behowell:avatar-icon

Conversation

@behowell

@behowell behowell commented Jun 15, 2021

Copy link
Copy Markdown
Contributor

Pull request checklist

  • Include a change request file using $ yarn change

Description of changes

  • Replace Avatar's old MDL2 person icon with the fluent version, and update priority order for displaying the icon: it will only be shown if there is no name/initials or image.
  • Update Avatar examples and VR tests to include new fluent icons
  • Move example badges into AvatarExamples in example-data, since the example badge values are just strings or simple objects.
  • Minor clean up to useAvatar's handling of the badge slot, so that useAvatar references PresenceBadge, rather than the Badge component

@behowell behowell requested a review from a team June 15, 2021 07:21
@behowell behowell requested a review from a team as a code owner June 15, 2021 07:21
ling1726
ling1726 reviewed Jun 15, 2021
View reviewed changes
Comment thread packages/react-avatar/package.json Outdated
},
"dependencies": {
"@fluentui/react-badge": "^9.0.0-alpha.46",
"@fluentui/react-icons": "^1.1.130",

@ling1726 ling1726 Jun 15, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should we discuss how best to introduce this dependency in our weekly sync ? If each component has a separate dependency on this icon package, it might be difficult for our consumers to use different packages and figure out what icons are being used in their app ?

There could potentially be a case to use it as peer dependencies too perhaps

@Hotell Hotell Jun 15, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

case to use it as peer dependencies too perhaps

peerDependencies sounds good to me! just keeps me wondering what approach would users need to take if they would like to use different set of icons? Provide them one by one per every component slots?

@Hotell Hotell Jun 15, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should we discuss how best to introduce this dependency in our weekly sync ?

that sounds like a very good idea -> output from that conversation should be written down as an RFC probably

@behowell behowell Jul 31, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@ling1726 @Hotell is it ok to check in this PR as-is, and when the dependencies issue is resolved, that can be handled in a future PR?

@Hotell Hotell Aug 4, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

sounds good @behowell

@size-auditor

size-auditor Bot commented Jun 15, 2021 •
edited
Loading

Copy link
Copy Markdown

Asset size changes

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

Baseline commit: 6a28e3e4cc5660f3b77f311564a132344816c751 (build)

@fabricteam

fabricteam commented Jun 15, 2021 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Perf Analysis (@fluentui/react)

No significant results to display.

All results

Scenario Render type Master Ticks PR Ticks Iterations Status
Avatar mount 781 908 5000
BaseButton mount 898 858 5000
Breadcrumb mount 2578 2605 1000
ButtonNext mount 427 414 5000
Checkbox mount 1494 1505 5000
CheckboxBase mount 1236 1264 5000
ChoiceGroup mount 4662 4642 5000
ComboBox mount 966 965 1000
CommandBar mount 10087 9923 1000
ContextualMenu mount 6047 6001 1000
DefaultButton mount 1132 1106 5000
DetailsRow mount 3692 3634 5000
DetailsRowFast mount 3730 3696 5000
DetailsRowNoStyles mount 3545 3506 5000
Dialog mount 2122 2156 1000
DocumentCardTitle mount 141 147 1000
Dropdown mount 3242 3143 5000
FluentProviderNext mount 7375 7556 5000
FocusTrapZone mount 1795 1749 5000
FocusZone mount 1796 1776 5000
IconButton mount 1707 1700 5000
Label mount 355 347 5000
Layer mount 1765 1802 5000
Link mount 460 468 5000
MakeStyles mount 1866 1877 50000
MenuButton mount 1432 1443 5000
MessageBar mount 1999 1991 5000
Nav mount 3282 3227 1000
OverflowSet mount 1053 1054 5000
Panel mount 2078 2057 1000
Persona mount 820 842 1000
Pivot mount 1391 1410 1000
PrimaryButton mount 1264 1278 5000
Rating mount 7548 7533 5000
SearchBox mount 1297 1256 5000
Shimmer mount 2528 2493 5000
Slider mount 1907 1965 5000
SpinButton mount 4876 4883 5000
Spinner mount 402 415 5000
SplitButton mount 3097 3141 5000
Stack mount 483 494 5000
StackWithIntrinsicChildren mount 1534 1542 5000
StackWithTextChildren mount 4508 4460 5000
SwatchColorPicker mount 10131 10113 5000
Tabs mount 1406 1405 1000
TagPicker mount 2576 2566 5000
TeachingBubble mount 11726 11683 5000
Text mount 398 418 5000
TextField mount 1356 1320 5000
ThemeProvider mount 1156 1163 5000
ThemeProvider virtual-rerender 605 596 5000
Toggle mount 806 827 5000
buttonNative mount 115 112 5000

Perf Analysis (@fluentui/react-northstar)

Perf tests with no regressions
Scenario Current PR Ticks Baseline Ticks Ratio
ChatWithPopoverPerf.default 375 343 1.09:1
DividerMinimalPerf.default 374 344 1.09:1
TextMinimalPerf.default 351 322 1.09:1
RefMinimalPerf.default 234 218 1.07:1
TableMinimalPerf.default 403 378 1.07:1
TreeMinimalPerf.default 809 766 1.06:1
AttachmentSlotsPerf.default 1069 1027 1.04:1
HeaderMinimalPerf.default 353 339 1.04:1
PopupMinimalPerf.default 597 574 1.04:1
SkeletonMinimalPerf.default 350 336 1.04:1
BoxMinimalPerf.default 348 338 1.03:1
ButtonSlotsPerf.default 525 511 1.03:1
FormMinimalPerf.default 389 377 1.03:1
ListNestedPerf.default 556 541 1.03:1
LoaderMinimalPerf.default 682 664 1.03:1
MenuMinimalPerf.default 835 807 1.03:1
AnimationMinimalPerf.default 407 398 1.02:1
CarouselMinimalPerf.default 455 444 1.02:1
DatepickerMinimalPerf.default 5433 5311 1.02:1
DialogMinimalPerf.default 745 728 1.02:1
DropdownMinimalPerf.default 3118 3057 1.02:1
HeaderSlotsPerf.default 744 732 1.02:1
ItemLayoutMinimalPerf.default 1221 1193 1.02:1
PortalMinimalPerf.default 180 177 1.02:1
ReactionMinimalPerf.default 373 365 1.02:1
SegmentMinimalPerf.default 344 338 1.02:1
AccordionMinimalPerf.default 163 161 1.01:1
AttachmentMinimalPerf.default 154 152 1.01:1
ButtonMinimalPerf.default 164 162 1.01:1
ChatDuplicateMessagesPerf.default 283 281 1.01:1
EmbedMinimalPerf.default 4081 4044 1.01:1
ListCommonPerf.default 617 608 1.01:1
ListMinimalPerf.default 509 505 1.01:1
MenuButtonMinimalPerf.default 1592 1584 1.01:1
CustomToolbarPrototype.default 3808 3780 1.01:1
TreeWith60ListItems.default 168 166 1.01:1
ButtonOverridesMissPerf.default 1685 1684 1:1
CardMinimalPerf.default 534 533 1:1
CheckboxMinimalPerf.default 2698 2706 1:1
ImageMinimalPerf.default 376 377 1:1
InputMinimalPerf.default 1249 1253 1:1
LayoutMinimalPerf.default 372 371 1:1
ListWith60ListItems.default 635 633 1:1
ProviderMergeThemesPerf.default 1660 1668 1:1
SplitButtonMinimalPerf.default 3751 3755 1:1
TableManyItemsPerf.default 1866 1871 1:1
TextAreaMinimalPerf.default 496 494 1:1
VideoMinimalPerf.default 582 584 1:1
AvatarMinimalPerf.default 191 192 0.99:1
ChatMinimalPerf.default 638 643 0.99:1
DropdownManyItemsPerf.default 658 665 0.99:1
LabelMinimalPerf.default 369 372 0.99:1
RadioGroupMinimalPerf.default 437 440 0.99:1
TooltipMinimalPerf.default 997 1006 0.99:1
GridMinimalPerf.default 328 335 0.98:1
ProviderMinimalPerf.default 988 1004 0.98:1
SliderMinimalPerf.default 1551 1581 0.98:1
StatusMinimalPerf.default 654 670 0.98:1
IconMinimalPerf.default 610 622 0.98:1
ToolbarMinimalPerf.default 897 926 0.97:1
FlexMinimalPerf.default 265 276 0.96:1
RosterPerf.default 1087 1149 0.95:1
AlertMinimalPerf.default 255 278 0.92:1

@ling1726

ling1726 commented Jun 15, 2021

Copy link
Copy Markdown
Contributor

Also an important question.... is this icon package tree shakeable ????!!!???

image

ling1726
ling1726 previously requested changes Jun 15, 2021
View reviewed changes

@ling1726 ling1726 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Blocking for bundle size gate scariness

@Hotell

Hotell commented Jun 15, 2021

Copy link
Copy Markdown
Contributor

Also an important question.... is this icon package tree shakeable ????!!!???

image

  • the package has no PURE pragmas specified nor sideEffects false thus I'd say webpack is clueless what can be tree shaken

https://unpkg.com/browse/@fluentui/[email protected]/lib/esm/components/AccessTime24Filled.js

@layershifter

layershifter commented Jun 21, 2021

Copy link
Copy Markdown
Member

Also an important question.... is this icon package tree shakeable ????!!!???

image

microsoft/fluentui-system-icons#282 adds sideEffects: false, let's check how it will impact this PR 😉

@msft-fluent-ui-bot

msft-fluent-ui-bot commented Jun 28, 2021

Copy link
Copy Markdown
Collaborator

This pull request has been automatically marked as stale because it was marked as requiring author feedback but has not had any activity for 7 days. It will be closed if no further activity occurs within 5 days of this comment. Thank you for your contributions to Fluent UI!

@behowell behowell reopened this Jul 6, 2021
This was referenced Jul 21, 2021
Closed
Closed
layershifter
layershifter reviewed Aug 2, 2021
View reviewed changes
Comment thread packages/react-avatar/src/components/Avatar/Avatar.types.ts Outdated
@behowell behowell requested a review from Hotell August 3, 2021 11:03
Hotell
Hotell reviewed Aug 3, 2021
View reviewed changes
Comment thread packages/react-avatar/src/components/Avatar/Avatar.types.ts Outdated
Comment thread packages/react-avatar/package.json
@behowell behowell mentioned this pull request Aug 4, 2021
Closed
Hotell
Hotell reviewed Aug 4, 2021
View reviewed changes
Comment thread packages/react-avatar/src/components/Avatar/Avatar.types.ts
@behowell behowell merged commit c0e62e6 into microsoft:master Aug 4, 2021
@behowell behowell deleted the avatar-icon branch August 4, 2021 19:18
@behowell behowell mentioned this pull request Aug 5, 2021
Merged
1 task
@msft-fluent-ui-bot

msft-fluent-ui-bot commented Aug 5, 2021

Copy link
Copy Markdown
Collaborator

🎉@fluentui/[email protected] has been released which incorporates this pull request.:tada:

Handy links:

@msft-fluent-ui-bot

msft-fluent-ui-bot commented Aug 5, 2021

Copy link
Copy Markdown
Collaborator

🎉@fluentui/[email protected] has been released which incorporates this pull request.:tada:

Handy links:

@msft-fluent-ui-bot

msft-fluent-ui-bot commented Aug 5, 2021

Copy link
Copy Markdown
Collaborator

🎉@fluentui/[email protected] has been released which incorporates this pull request.:tada:

Handy links:

@msft-fluent-ui-bot

msft-fluent-ui-bot commented Aug 5, 2021

Copy link
Copy Markdown
Collaborator

🎉@fluentui/[email protected] has been released which incorporates this pull request.:tada:

Handy links:

@msft-fluent-ui-bot

msft-fluent-ui-bot commented Aug 5, 2021

Copy link
Copy Markdown
Collaborator

🎉@fluentui/[email protected] has been released which incorporates this pull request.:tada:

Handy links:

@msft-fluent-ui-bot

msft-fluent-ui-bot commented Aug 5, 2021

Copy link
Copy Markdown
Collaborator

🎉@fluentui/[email protected] has been released which incorporates this pull request.:tada:

Handy links:

@msft-fluent-ui-bot

msft-fluent-ui-bot commented Aug 5, 2021

Copy link
Copy Markdown
Collaborator

🎉@fluentui/[email protected] has been released which incorporates this pull request.:tada:

Handy links:

@msft-fluent-ui-bot

msft-fluent-ui-bot commented Aug 5, 2021

Copy link
Copy Markdown
Collaborator

🎉@fluentui/[email protected] has been released which incorporates this pull request.:tada:

Handy links:

@msft-fluent-ui-bot

msft-fluent-ui-bot commented Aug 5, 2021

Copy link
Copy Markdown
Collaborator

🎉@fluentui/[email protected] has been released which incorporates this pull request.:tada:

Handy links:

@msft-fluent-ui-bot

msft-fluent-ui-bot commented Aug 5, 2021

Copy link
Copy Markdown
Collaborator

🎉@fluentui/[email protected] has been released which incorporates this pull request.:tada:

Handy links:

@msft-fluent-ui-bot

msft-fluent-ui-bot commented Aug 5, 2021

Copy link
Copy Markdown
Collaborator

🎉@fluentui/[email protected] has been released which incorporates this pull request.:tada:

Handy links:

@ling1726 ling1726 mentioned this pull request Aug 9, 2021
Merged
2 tasks
@spmonahan spmonahan mentioned this pull request Apr 1, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Hotell Hotell Hotell approved these changes
@ecraig12345 ecraig12345 ecraig12345 left review comments
@khmakoto khmakoto khmakoto approved these changes
@layershifter layershifter layershifter approved these changes
+1 more reviewer
@ling1726 ling1726 ling1726 left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

@behowell behowell

Labels

Component: Avatar Effort II: Days Fluent UI react-components (v9)

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

9 participants

@behowell @fabricteam @ling1726 @Hotell @layershifter @msft-fluent-ui-bot @ecraig12345 @khmakoto @miroslavstastny