-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
(web-components): add avatar as a new Fluent 2 aligned component #26729
(web-components): add avatar as a new Fluent 2 aligned component #26729
Conversation
d732376
to
2680450
Compare
📊 Bundle size report🤖 This report was generated against 7d9573d73b4be010c2b07540fa0c23c27aba2821 |
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 34ea6e8:
|
c00523f
to
0f44ff9
Compare
Asset size changesSize Auditor did not detect a change in bundle size for any component! Baseline commit: 7d9573d73b4be010c2b07540fa0c23c27aba2821 (build) |
0f44ff9
to
0e1a331
Compare
:host([shape='square'][size='96']), | ||
:host([shape='square'][size='120']), | ||
:host([shape='square'][size='128']) { | ||
border-radius: ${borderRadiusXLarge}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be nice to get docs up with all these values for reference.
630fab7
to
021cf99
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approving for v-build owned files.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think all the *Changed
callbacks which call either setInitials()
or setColor()
are unnecessary asboth are just compute
functions which do not alter anything.
When any of name
, initials
, color
or color-id
attributes changes, the relevant part of the template is invalidated and re-rendered.
change/@fluentui-web-components-1c73e6e8-61a0-46c8-b69e-0820f5a2488d.json
Outdated
Show resolved
Hide resolved
* HTML Attribute: active | ||
*/ | ||
@attr | ||
public active?: AvatarActive | undefined; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Besides active
and inactive
, FUIR9 also uses unset
(which is default).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@miroslavstastny this is one where I think we need to figure out the right thing...am I mistaken that the intent of "unset" is undefined? Basically "removing" the state itself? While I'm more than happy to align on including "unset", it's basically a noop here, and our approach was to not provide default values to attributes when possible to avoid cluttering DOM. Happy to align, but I think these are places where I'm a bit lost on why certain decisions were made.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is the only outstanding instance, I'm going to look for detail on this in past issues.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, I've found what I think is precedent from FUIR9 for adopting undefined
here instead of "unset". This specific instance seems to be a large part of prompting this RFC: #23304. Given we're doing new work here, I'd rather align with this forward-thinking direction which I assume will be adopted into the React Avatar implementation when we can next make a breaking change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense, let's go with undefined
. Do you plan to add a spec documenting the differences between FUIR9 and WC3?
0c0c88f
to
c56866b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unblocking the PR, I still would like to understand colorIdChanged()
Removed/resolved - I think I might have misspelled or typed a value which doesn't exist. 👀 |
0ff51f6
to
4006a9f
Compare
) * add avatar as a new component * add export path * update helpers path in avatar stories * create public static value for named colors * update styles to support any slotted badge * add size 16 styles and stories * update animations to use tokens * add support for prefers reduced motion on active and inactive transitions * change set to generate for color and initials fns * add large badge styles * remove unnecessary badge cchange handlers * fix brand color in dark mode with static inverted value * update change file * remove unnecessary lifecycle event * add avatar readme for react deltas * update attributes as optional * update api report
…rosoft#26729) * add avatar as a new component * add export path * update helpers path in avatar stories * create public static value for named colors * update styles to support any slotted badge * add size 16 styles and stories * update animations to use tokens * add support for prefers reduced motion on active and inactive transitions * change set to generate for color and initials fns * add large badge styles * remove unnecessary badge cchange handlers * fix brand color in dark mode with static inverted value * update change file * remove unnecessary lifecycle event * add avatar readme for react deltas * update attributes as optional * update api report
…rosoft#26729) * add avatar as a new component * add export path * update helpers path in avatar stories * create public static value for named colors * update styles to support any slotted badge * add size 16 styles and stories * update animations to use tokens * add support for prefers reduced motion on active and inactive transitions * change set to generate for color and initials fns * add large badge styles * remove unnecessary badge cchange handlers * fix brand color in dark mode with static inverted value * update change file * remove unnecessary lifecycle event * add avatar readme for react deltas * update attributes as optional * update api report
…rosoft#26729) * add avatar as a new component * add export path * update helpers path in avatar stories * create public static value for named colors * update styles to support any slotted badge * add size 16 styles and stories * update animations to use tokens * add support for prefers reduced motion on active and inactive transitions * change set to generate for color and initials fns * add large badge styles * remove unnecessary badge cchange handlers * fix brand color in dark mode with static inverted value * update change file * remove unnecessary lifecycle event * add avatar readme for react deltas * update attributes as optional * update api report
) * add avatar as a new component * add export path * update helpers path in avatar stories * create public static value for named colors * update styles to support any slotted badge * add size 16 styles and stories * update animations to use tokens * add support for prefers reduced motion on active and inactive transitions * change set to generate for color and initials fns * add large badge styles * remove unnecessary badge cchange handlers * fix brand color in dark mode with static inverted value * update change file * remove unnecessary lifecycle event * add avatar readme for react deltas * update attributes as optional * update api report
) * add avatar as a new component * add export path * update helpers path in avatar stories * create public static value for named colors * update styles to support any slotted badge * add size 16 styles and stories * update animations to use tokens * add support for prefers reduced motion on active and inactive transitions * change set to generate for color and initials fns * add large badge styles * remove unnecessary badge cchange handlers * fix brand color in dark mode with static inverted value * update change file * remove unnecessary lifecycle event * add avatar readme for react deltas * update attributes as optional * update api report
) * add avatar as a new component * add export path * update helpers path in avatar stories * create public static value for named colors * update styles to support any slotted badge * add size 16 styles and stories * update animations to use tokens * add support for prefers reduced motion on active and inactive transitions * change set to generate for color and initials fns * add large badge styles * remove unnecessary badge cchange handlers * fix brand color in dark mode with static inverted value * update change file * remove unnecessary lifecycle event * add avatar readme for react deltas * update attributes as optional * update api report
) * add avatar as a new component * add export path * update helpers path in avatar stories * create public static value for named colors * update styles to support any slotted badge * add size 16 styles and stories * update animations to use tokens * add support for prefers reduced motion on active and inactive transitions * change set to generate for color and initials fns * add large badge styles * remove unnecessary badge cchange handlers * fix brand color in dark mode with static inverted value * update change file * remove unnecessary lifecycle event * add avatar readme for react deltas * update attributes as optional * update api report
) * add avatar as a new component * add export path * update helpers path in avatar stories * create public static value for named colors * update styles to support any slotted badge * add size 16 styles and stories * update animations to use tokens * add support for prefers reduced motion on active and inactive transitions * change set to generate for color and initials fns * add large badge styles * remove unnecessary badge cchange handlers * fix brand color in dark mode with static inverted value * update change file * remove unnecessary lifecycle event * add avatar readme for react deltas * update attributes as optional * update api report
) * add avatar as a new component * add export path * update helpers path in avatar stories * create public static value for named colors * update styles to support any slotted badge * add size 16 styles and stories * update animations to use tokens * add support for prefers reduced motion on active and inactive transitions * change set to generate for color and initials fns * add large badge styles * remove unnecessary badge cchange handlers * fix brand color in dark mode with static inverted value * update change file * remove unnecessary lifecycle event * add avatar readme for react deltas * update attributes as optional * update api report
) * add avatar as a new component * add export path * update helpers path in avatar stories * create public static value for named colors * update styles to support any slotted badge * add size 16 styles and stories * update animations to use tokens * add support for prefers reduced motion on active and inactive transitions * change set to generate for color and initials fns * add large badge styles * remove unnecessary badge cchange handlers * fix brand color in dark mode with static inverted value * update change file * remove unnecessary lifecycle event * add avatar readme for react deltas * update attributes as optional * update api report
) * add avatar as a new component * add export path * update helpers path in avatar stories * create public static value for named colors * update styles to support any slotted badge * add size 16 styles and stories * update animations to use tokens * add support for prefers reduced motion on active and inactive transitions * change set to generate for color and initials fns * add large badge styles * remove unnecessary badge cchange handlers * fix brand color in dark mode with static inverted value * update change file * remove unnecessary lifecycle event * add avatar readme for react deltas * update attributes as optional * update api report
) * add avatar as a new component * add export path * update helpers path in avatar stories * create public static value for named colors * update styles to support any slotted badge * add size 16 styles and stories * update animations to use tokens * add support for prefers reduced motion on active and inactive transitions * change set to generate for color and initials fns * add large badge styles * remove unnecessary badge cchange handlers * fix brand color in dark mode with static inverted value * update change file * remove unnecessary lifecycle event * add avatar readme for react deltas * update attributes as optional * update api report
Previous Behavior
We did not have a Fluent 2 aligned Avatar Component.
New Behavior
Now we do! closes #26672
This PR provides pretty much parity of API but proposes a flat structure to limit the size of our element by leveraging the default slot for string attributes. Additionally, I'm proposing a property/attribute delta -
color-id
rather thanid-for-color
, but I'm open to changing it back to align here.Lastly, this does not include the manipulation of aria-label and aria-labelledby automatically. There are a couple of considerations I think we should look at given the potential perf cost of "covering the sins" of consuming teams. If we went the guidance method, teams would construct their labels (we can provide specific examples!) and the cost is basically free as we're leveraging the native API's on the element. If we want to provide this by default then we need to observe and manage the lifecycle of both aria-label and aria-labelledby as well as observe the badge slot and then likely manipulate or modify slotted elements such as the badge slot as well. Finally, we'd need a method of providing a localized string to create this I think...which is easy enough with a formatter - but it encourages both us handling it and encourages us taking on the cost. Miro and I discussed this in our sync this week and agreed it isn't a blocker but I do think it's worth discussing the trade offs.
Related Issue(s)