-
Notifications
You must be signed in to change notification settings - Fork 2.9k
RFC: Reusing Avatar in other components #24790
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
Conversation
This RFC presents the problem of Avatar reuse when trying to follow design guidelines. Several solutions are proposed along with their pros/cons
📊 Bundle size report🤖 This report was generated against 48df172dd62d2bc71409c8cdb3967ea500dcd999 |
|
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 2feea25:
|
Asset size changesSize Auditor did not detect a change in bundle size for any component! Baseline commit: c37c8714056e1e021ac7da86ca4bec3428e1953a (build) |
|
|
||
| - Extra code in base component | ||
| - Needs prototyping and investigation | ||
| - All avatars under this context will be affected |
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 suggest to add example with Popover there - an Avatar inside a Popover will have size "20" instead of default one
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.
As I implemented in the prototype, the choice of slot to apply this context can be more 'intelligent' #24807, and honestly it might not be that bad 🤔
Co-authored-by: Oleksandr Fediashov <[email protected]>
…fluentui into rfc/avatar-icon-sizing
|
I believe the Shared context would be the best solution described here so far. Pros
Cons
|
| - Cannot ship design updates i.e. users will need to upgrade all usages if design decides that "64px" will be new size | ||
| - Extra work for users | ||
|
|
||
| ### Style override |
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.
Have we considered the usage of CSS variables for the style overrides proposal? This solves the "Does not handle props" con. If the size prop of the avatar adds styles with the CSS variable, it'll have priority over the overriden style by the Table.
A con to this approach is to document well the CSS variable.
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.
The problem with style overrides (I tried it this morning) is that for Avatar the size actually controls multiple css properties:
- width
- height
- icon slot size
- font size
- font weight
I don't think a single css var will actually help and it's a problem that @behowell highlighted offline during tech sync. Furthermore, if designers decide to update the styles with size, it's harder to know what properties to have to update
|
I am for the React context option 👍 |
* RFC: Reusing Avatar in other components This RFC presents the problem of Avatar reuse when trying to follow design guidelines. Several solutions are proposed along with their pros/cons * update * update * Update rfcs/react-components/convergence/reusing-avatar-in-components.md Co-authored-by: Oleksandr Fediashov <[email protected]> * add prototype implemenentation of context solution * update code sample * update preferred solution Co-authored-by: Oleksandr Fediashov <[email protected]>
This RFC presents the problem of Avatar reuse when trying to follow design guidelines. Several solutions are proposed along with their pros/cons
PREVIEW