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.

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

feat: adding Avatar component #20151

Closed

Conversation

khamudom
Copy link
Contributor

@khamudom khamudom commented Oct 7, 2021

Pull request checklist

  • Addresses an existing issue: Fixes #0000
  • Include a change request file using $ yarn change

Description of changes

Added Avatar component and styles.
Created examples in stories with the ability to change shape and toggle the badge on and off.

image

keyboard focus visual
image

@fabricteam
Copy link
Collaborator

fabricteam commented Oct 7, 2021

📊 Bundle size report

🤖 This report was generated against 8555daca54fa0ab490b5400b8d67a9430be9e8e5

@codesandbox-ci
Copy link

codesandbox-ci bot commented Oct 7, 2021

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit e65391b:

Sandbox Source
@fluentui/react 8 starter Configuration
@fluentui/react-components 9 starter Configuration

@size-auditor
Copy link

size-auditor bot commented Oct 7, 2021

Asset size changes

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

Baseline commit: 8555daca54fa0ab490b5400b8d67a9430be9e8e5 (build)

@fabricteam
Copy link
Collaborator

fabricteam commented Oct 7, 2021

Perf Analysis (@fluentui/react)

No significant results to display.

All results

Scenario Render type Master Ticks PR Ticks Iterations Status
Avatar mount 1067 1021 5000
BaseButton mount 1059 1044 5000
Breadcrumb mount 2807 2772 1000
ButtonNext mount 541 550 5000
Checkbox mount 1703 1770 5000
CheckboxBase mount 1498 1495 5000
ChoiceGroup mount 5376 5364 5000
ComboBox mount 1066 1101 1000
CommandBar mount 11088 11088 1000
ContextualMenu mount 6844 7128 1000
DefaultButton mount 1288 1289 5000
DetailsRow mount 4147 4154 5000
DetailsRowFast mount 4149 4097 5000
DetailsRowNoStyles mount 3947 4007 5000
Dialog mount 2804 2744 1000
DocumentCardTitle mount 186 196 1000
Dropdown mount 3574 3554 5000
FluentProviderNext mount 3436 3462 5000
FluentProviderWithTheme mount 224 227 10
FluentProviderWithTheme virtual-rerender 124 126 10
FluentProviderWithTheme virtual-rerender-with-unmount 252 280 10
FocusTrapZone mount 1949 1927 5000
FocusZone mount 1921 1900 5000
IconButton mount 2014 1959 5000
Label mount 388 388 5000
Layer mount 3272 3292 5000
Link mount 522 533 5000
MakeStyles mount 1889 1941 50000
MenuButton mount 1627 1627 5000
MessageBar mount 2136 2261 5000
Nav mount 3610 3631 1000
OverflowSet mount 1314 1156 5000
Panel mount 2656 2676 1000
Persona mount 916 905 1000
Pivot mount 1555 1598 1000
PrimaryButton mount 1437 1461 5000
Rating mount 8738 8715 5000
SearchBox mount 1492 1505 5000
Shimmer mount 3052 2816 5000
Slider mount 2120 2138 5000
SpinButton mount 5465 5492 5000
Spinner mount 469 475 5000
SplitButton mount 3476 3500 5000
Stack mount 588 607 5000
StackWithIntrinsicChildren mount 1907 1936 5000
StackWithTextChildren mount 5237 5277 5000
SwatchColorPicker mount 11710 11459 5000
Tabs mount 1567 1560 1000
TagPicker mount 2896 2939 5000
TeachingBubble mount 14060 13780 5000
Text mount 492 480 5000
TextField mount 1600 1563 5000
ThemeProvider mount 1285 1257 5000
ThemeProvider virtual-rerender 652 659 5000
ThemeProvider virtual-rerender-with-unmount 2070 2090 5000
Toggle mount 916 921 5000
buttonNative mount 138 149 5000

Perf Analysis (@fluentui/react-northstar)

Perf tests with no regressions
Scenario Current PR Ticks Baseline Ticks Ratio
AttachmentMinimalPerf.default 209 177 1.18:1
TreeWith60ListItems.default 232 197 1.18:1
ButtonMinimalPerf.default 220 188 1.17:1
DividerMinimalPerf.default 414 369 1.12:1
GridMinimalPerf.default 388 356 1.09:1
ListWith60ListItems.default 748 689 1.09:1
LoaderMinimalPerf.default 793 728 1.09:1
ReactionMinimalPerf.default 456 420 1.09:1
FlexMinimalPerf.default 336 310 1.08:1
LayoutMinimalPerf.default 428 398 1.08:1
BoxMinimalPerf.default 404 376 1.07:1
CarouselMinimalPerf.default 531 496 1.07:1
TableMinimalPerf.default 468 442 1.06:1
TextMinimalPerf.default 404 380 1.06:1
AccordionMinimalPerf.default 170 162 1.05:1
ButtonOverridesMissPerf.default 2008 1919 1.05:1
ChatWithPopoverPerf.default 435 416 1.05:1
ToolbarMinimalPerf.default 1096 1043 1.05:1
VideoMinimalPerf.default 698 665 1.05:1
AlertMinimalPerf.default 323 311 1.04:1
ImageMinimalPerf.default 427 411 1.04:1
MenuButtonMinimalPerf.default 1853 1790 1.04:1
RefMinimalPerf.default 253 243 1.04:1
SegmentMinimalPerf.default 400 385 1.04:1
AvatarMinimalPerf.default 243 235 1.03:1
CheckboxMinimalPerf.default 2988 2904 1.03:1
DatepickerMinimalPerf.default 6077 5876 1.03:1
ItemLayoutMinimalPerf.default 1372 1333 1.03:1
TableManyItemsPerf.default 2214 2148 1.03:1
DialogMinimalPerf.default 876 856 1.02:1
TooltipMinimalPerf.default 1176 1151 1.02:1
CardMinimalPerf.default 649 643 1.01:1
LabelMinimalPerf.default 434 428 1.01:1
ListNestedPerf.default 650 643 1.01:1
MenuMinimalPerf.default 954 946 1.01:1
RadioGroupMinimalPerf.default 500 493 1.01:1
SliderMinimalPerf.default 1856 1838 1.01:1
StatusMinimalPerf.default 758 749 1.01:1
IconMinimalPerf.default 714 707 1.01:1
TextAreaMinimalPerf.default 576 572 1.01:1
ChatMinimalPerf.default 723 723 1:1
DropdownManyItemsPerf.default 780 783 1:1
SkeletonMinimalPerf.default 402 404 1:1
CustomToolbarPrototype.default 4477 4475 1:1
TreeMinimalPerf.default 864 868 1:1
DropdownMinimalPerf.default 3347 3385 0.99:1
EmbedMinimalPerf.default 4561 4606 0.99:1
HeaderSlotsPerf.default 829 834 0.99:1
RosterPerf.default 1329 1347 0.99:1
ProviderMinimalPerf.default 1263 1277 0.99:1
SplitButtonMinimalPerf.default 4729 4788 0.99:1
ButtonSlotsPerf.default 625 638 0.98:1
HeaderMinimalPerf.default 404 411 0.98:1
InputMinimalPerf.default 1399 1422 0.98:1
ListCommonPerf.default 711 724 0.98:1
ListMinimalPerf.default 582 596 0.98:1
PortalMinimalPerf.default 181 184 0.98:1
ProviderMergeThemesPerf.default 1797 1826 0.98:1
AttachmentSlotsPerf.default 1179 1215 0.97:1
AnimationMinimalPerf.default 448 465 0.96:1
FormMinimalPerf.default 454 472 0.96:1
PopupMinimalPerf.default 620 656 0.95:1
ChatDuplicateMessagesPerf.default 321 340 0.94:1

@brookdozer
Copy link
Contributor

./fixtures/avatar.html isn't needed, though it doesn't hurt to leave it.

@khamudom khamudom force-pushed the users/khamu/add-avatar-component branch from e5ec860 to 49dccfa Compare October 10, 2021 00:46
Copy link
Member

@chrisdholt chrisdholt left a comment

Choose a reason for hiding this comment

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

I'd like us to dig in on the sizing approach here - it seems rather unintuitive and cumbersome. I think a recipe for the default approach is fine, but I'm not understanding the link if I want to update with updating via a different token. That seems odd.

@chrisdholt chrisdholt assigned chrisdholt and unassigned bsunderhus Oct 13, 2021
@litteredwitherrors
Copy link
Contributor

Getting weird rendering on the Avatar story page.

image

@brookdozer
Copy link
Contributor

Getting weird rendering on the Avatar story page.

image

This is because the switches use a fluent-badge, which you are styling. I have a fix to remove those badges coming soon so please ignore for now.

@khamudom
Copy link
Contributor Author

Getting weird rendering on the Avatar story page.
image

This is because the switches use a fluent-badge, which you are styling. I have a fix to remove those badges coming soon so please ignore for now.

Try syncing to the latest PR, that was addressed.

@chrisdholt
Copy link
Member

@khamudom focus styles look much better - I am curious though, are there :hover styles for instances which have an link here? I'm not seeing any visual delta beyond just the pointer and that is likely going to be problematic. What possible solutions do we have for hover states? Perhaps @bheston has some thoughts here? I don't think we can ship this without a hover state.

@EisenbergEffect
Copy link
Contributor

@chrisdholt What's the plan for this? It's 5 months old.

@bheston
Copy link
Contributor

bheston commented Mar 9, 2022

@chrisdholt What's the plan for this? It's 5 months old.

Sorry, this is probably holding on me. I was hoping to pull together a sample of Avatars from around the web, but my main concern was around the last comment Chris tagged me in. I don't understand why Avatar has its own link attribute. I understand this would be a foundation change (deprecation), and it is optional, but it seems that Avatar itself should just be the picture, or paired with the presence badge or any other combination someone can imagine, but if it needs to link somewhere it should just be inside an Anchor which already has the correct styling. There are so many variations even across GitHub, Discord, Teams, Edge, Office, etc. that combining the visual and the functional is more complex. This component seems stronger with less functionality.

My proposal is to deprecate link and not implement it in Fluent UI. This aligns with what I've seen in the Fluent UI design as well, where the only hover treatment documented is a tooltip or flyout. Our existing components already support this functionality without visual styling interactions.

@chrisdholt
Copy link
Member

@chrisdholt What's the plan for this? It's 5 months old.

Sorry, this is probably holding on me. I was hoping to pull together a sample of Avatars from around the web, but my main concern was around the last comment Chris tagged me in. I don't understand why Avatar has its own link attribute. I understand this would be a foundation change (deprecation), and it is optional, but it seems that Avatar itself should just be the picture, or paired with the presence badge or any other combination someone can imagine, but if it needs to link somewhere it should just be inside an Anchor which already has the correct styling. There are so many variations even across GitHub, Discord, Teams, Edge, Office, etc. that combining the visual and the functional is more complex. This component seems stronger with less functionality.

My proposal is to deprecate link and not implement it in Fluent UI. This aligns with what I've seen in the Fluent UI design as well, where the only hover treatment documented is a tooltip or flyout. Our existing components already support this functionality without visual styling interactions.

Thanks @bheston - on deprecation of link, that's fine and helpful considering where we are in the process. For tooltips, I think that's fine, though I'm curious how we would attach behavior for something like a flyout, where invoking via keyboard will need to move a user there somehow. Flyouts have a lot of interactive content in them so would we have an avatar that would primarily be a button? If we can grab what works for most flyouts and pri that in FAST we can do what's necessary in Fluent as divergent, not too worried about that. My primary concern is making sure that we represent most flyouts here.

@bheston
Copy link
Contributor

bheston commented Mar 9, 2022

@chrisdholt What's the plan for this? It's 5 months old.

Sorry, this is probably holding on me. I was hoping to pull together a sample of Avatars from around the web, but my main concern was around the last comment Chris tagged me in. I don't understand why Avatar has its own link attribute. I understand this would be a foundation change (deprecation), and it is optional, but it seems that Avatar itself should just be the picture, or paired with the presence badge or any other combination someone can imagine, but if it needs to link somewhere it should just be inside an Anchor which already has the correct styling. There are so many variations even across GitHub, Discord, Teams, Edge, Office, etc. that combining the visual and the functional is more complex. This component seems stronger with less functionality.
My proposal is to deprecate link and not implement it in Fluent UI. This aligns with what I've seen in the Fluent UI design as well, where the only hover treatment documented is a tooltip or flyout. Our existing components already support this functionality without visual styling interactions.

Thanks @bheston - on deprecation of link, that's fine and helpful considering where we are in the process. For tooltips, I think that's fine, though I'm curious how we would attach behavior for something like a flyout, where invoking via keyboard will need to move a user there somehow. Flyouts have a lot of interactive content in them so would we have an avatar that would primarily be a button? If we can grab what works for most flyouts and pri that in FAST we can do what's necessary in Fluent as divergent, not too worried about that. My primary concern is making sure that we represent most flyouts here.

Good catch. Fluent describes it as a "popover", which looks like some combination of a tooltip, flyout, and teaching tip. If there is no interactive content I suspect it would function like a tooltip, but if there is something interactive, you're right it would need to get focus. In that case from an accessibility perspective the avatar would probably be wrapped in a button instead, which we can support. Either way, I don't think that would be the responsibility of the Avatar either, but instead a composition within Button.

Copy link
Contributor

@EisenbergEffect EisenbergEffect left a comment

Choose a reason for hiding this comment

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

See comments.

@msft-fluent-ui-bot
Copy link
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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.