From acc227540867ea2bdc572d799f6c9e94b0d56821 Mon Sep 17 00:00:00 2001 From: Lingfan Gao Date: Wed, 14 Sep 2022 15:02:05 +0200 Subject: [PATCH 1/7] 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 --- .../reusing-avatar-in-components.md | 257 ++++++++++++++++++ 1 file changed, 257 insertions(+) create mode 100644 rfcs/react-components/convergence/reusing-avatar-in-components.md diff --git a/rfcs/react-components/convergence/reusing-avatar-in-components.md b/rfcs/react-components/convergence/reusing-avatar-in-components.md new file mode 100644 index 00000000000000..69678f7909d04d --- /dev/null +++ b/rfcs/react-components/convergence/reusing-avatar-in-components.md @@ -0,0 +1,257 @@ +# RFC: Reusing Avatar in other components + +@ling1726 + +## Summary + +This RFC enumerates the possible ways to handle: + +- Avatar default props +- Avatar default sizing + +When the component is reused within other v9 components. There is a broader question of how to +internally reuse components in general, hopefully resolving the problem described in this RFC +will help us make easier decisions for component reuse in the future. + +## Background + +Fluent UI v9 has design requirements on each of its components. These requirements are compiled from product +usage. This is a good thing since our components can be easily used to power products. However one of the consequences +of stricter design requirements it the resuse of our own components. Sometimes the component reusage can involve +different defaults. How can we handle this in React Library? + +## Problem statement + +Let's use `Avatar` as an example. We have concrete problems here that involve the `Table` and `Persona` components. +Both of these components use `Avatar` and require a default size when used in those components. + +```tsx + + + + + {/* The Avatar should have size 20 to follow the Table size */} + }>Main content + + + +
+``` + +The default size for the `Avatar` component is `32`. This means that the component that is reusing the avatar needs +to do some extra work to make sure that the avatar follows design guidance. + +## Detailed Design or Proposal + +### Userland problem + +We could treat this as a userland problem, and require that apps/features follow design guidance when using an +avatar with these components. + +```tsx + + + + + {/* Explicitly set the size */} + }>Main content + + + +
+``` + +#### Pros + +- No extra logic in Fluent code + +#### Cons + +- Users must know about design guidance +- Extra work for users + +### Style override + +This is a method that is already done to support a similar problem for icons in: + +- PresenceBadge +- Combobox + +The component should use selectors to target the contents of the slot and force the size. In the case of icons +a simple `svg` selector would work + +```tsx +const useStyles = makeStyles({ + iconSlot: { + '& svg': { + width: '10px', + height: '10px', + }, + }, +}); +``` + +However when using this method with other components we would use the className selectors: + +```tsx +const useStyles = makeStyles({ + avatarSlot: { + '& fui-Avatar': { + width: '10px', + height: '10px', + }, + }, +}); +``` + +#### Pros + +- Users can use base components normally +- Override can be scoped to the single slot -> doesn't affect other slots + +#### Cons + +- Harder for users to override styles since selectors are more specific +- Not obvious for users how base component adjusts to the parent component - magic + +### Recompose components + +This solution is used in `Toolbar` where `ToolbarButton` is recomposed from the normal `Button` + +```tsx +const tableAvatarSizeMap = { + small: 24, + smaller: 20, +}; + +export function TableAvatar: React.FC(props) { + // recomposed TableAvatar can read TableContext to adapt to its size prop + const { size } = useTableContext(); + + const state = useAvatar({...props, size: tableAvatarSizeMap[size]}); + state = useAvatarStyles(props); + return renderAvatar(state); +} +``` + +```tsx + + + + + {/* This will be sized automtically */} + }>Main content + + + +
+``` + +#### Pros + +- Best level of support for component reuse +- Can control everything from style override to props + +#### Cons + +- Lots of variations of a base component, TableAvatar, PersonaAvatar, ToolarAvatar... +- How do users know they should use a 'special' variant of the component? + +### Separate slots + +This solution is what the `Alert` currently does (however, it does not seem necessary in that component): + +```tsx +export function useTableCellLayout() { + const state = { + components: { + icon: 'span', + avatar: Avatar, + } + avatar: resolveShorthand(props.avatar); + } + + /** Avatar prop takes precedence over the icon*/ + if (!avatar) { + icon = resolveShorthand(props.icon, { + defaultProps: { + children: defaultIcon, + }, + }); + state.icon = icon; + } + + return state; +} + +export function renderTableCellLayout() { + +} +``` + +```tsx + + + + + }>Main content + + + {/* Internally the slot will override the size */} + Main content + + + +
+``` + +#### Pros + +- Very obvious to know what 'media' components are supported +- Can configure the component through our well defined (and typesafe) shorthand API + +#### Cons + +- Bundle size - users who don't want to use avatar will have it in their bundle +- Confusing priorities - One slot wins over the other, it's why we have `appearance` prop + +### Shared context + +We could consider a `AvatarContext` that is a part of `react-shared-contexts` that can be used by components that wish +to reuse the `Avatar`. + +```tsx +const tableAvatarSizeMap = { + small: 24, + smaller: 20, +}; + +export function renderTable(state) { + const { size } = state; + const { slots, slotProps } = getSlots(state); + + return ( + + {slotProps.root.children} + + ); +} +``` + +#### Pros + +- We also use context for lots of other user cases - it works + +#### Cons + +- Needs prototyping and investigation +- All avatars under this context will be affected +- This might not scale - AvatarContext, IconContext... + +## Discarded Solutions + +> TODO put discarded solutions here + +## Open Issues + + From 367245fde55aff223a01b7d8e82cf00db5db7494 Mon Sep 17 00:00:00 2001 From: Lingfan Gao Date: Wed, 14 Sep 2022 15:07:19 +0200 Subject: [PATCH 2/7] update --- .../react-components/convergence/reusing-avatar-in-components.md | 1 + 1 file changed, 1 insertion(+) diff --git a/rfcs/react-components/convergence/reusing-avatar-in-components.md b/rfcs/react-components/convergence/reusing-avatar-in-components.md index 69678f7909d04d..bd1ee8bd2458ed 100644 --- a/rfcs/react-components/convergence/reusing-avatar-in-components.md +++ b/rfcs/react-components/convergence/reusing-avatar-in-components.md @@ -111,6 +111,7 @@ const useStyles = makeStyles({ #### Cons +- Does not handle props - Harder for users to override styles since selectors are more specific - Not obvious for users how base component adjusts to the parent component - magic From 0fa80d2bf36144787eb18a4008e839fa0f43de7d Mon Sep 17 00:00:00 2001 From: Lingfan Gao Date: Wed, 14 Sep 2022 15:08:17 +0200 Subject: [PATCH 3/7] update --- .../react-components/convergence/reusing-avatar-in-components.md | 1 + 1 file changed, 1 insertion(+) diff --git a/rfcs/react-components/convergence/reusing-avatar-in-components.md b/rfcs/react-components/convergence/reusing-avatar-in-components.md index bd1ee8bd2458ed..38b6fc0fe8059b 100644 --- a/rfcs/react-components/convergence/reusing-avatar-in-components.md +++ b/rfcs/react-components/convergence/reusing-avatar-in-components.md @@ -245,6 +245,7 @@ export function renderTable(state) { #### Cons +- Extra code in base component - Needs prototyping and investigation - All avatars under this context will be affected - This might not scale - AvatarContext, IconContext... From 2d4cfcf688dc13bd5f0e44133e0ee8ddd5803ad3 Mon Sep 17 00:00:00 2001 From: ling1726 Date: Wed, 14 Sep 2022 16:04:32 +0200 Subject: [PATCH 4/7] Update rfcs/react-components/convergence/reusing-avatar-in-components.md Co-authored-by: Oleksandr Fediashov --- .../react-components/convergence/reusing-avatar-in-components.md | 1 + 1 file changed, 1 insertion(+) diff --git a/rfcs/react-components/convergence/reusing-avatar-in-components.md b/rfcs/react-components/convergence/reusing-avatar-in-components.md index 38b6fc0fe8059b..35941ca5c5cf78 100644 --- a/rfcs/react-components/convergence/reusing-avatar-in-components.md +++ b/rfcs/react-components/convergence/reusing-avatar-in-components.md @@ -68,6 +68,7 @@ avatar with these components. #### Cons - Users must know about design guidance +- 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 From 20d6966135db43d0e062298495b6383de473f21b Mon Sep 17 00:00:00 2001 From: Lingfan Gao Date: Thu, 15 Sep 2022 09:46:20 +0200 Subject: [PATCH 5/7] add prototype implemenentation of context solution --- .../convergence/reusing-avatar-in-components.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/rfcs/react-components/convergence/reusing-avatar-in-components.md b/rfcs/react-components/convergence/reusing-avatar-in-components.md index 38b6fc0fe8059b..fa2379325f9306 100644 --- a/rfcs/react-components/convergence/reusing-avatar-in-components.md +++ b/rfcs/react-components/convergence/reusing-avatar-in-components.md @@ -221,6 +221,8 @@ export function renderTableCellLayout() { We could consider a `AvatarContext` that is a part of `react-shared-contexts` that can be used by components that wish to reuse the `Avatar`. +This solution is prototyped in [#24807](https://github.com/microsoft/fluentui/pull/24807) + ```tsx const tableAvatarSizeMap = { small: 24, @@ -242,6 +244,7 @@ export function renderTable(state) { #### Pros - We also use context for lots of other user cases - it works +- Can be targeted to the speific slot that needs these overrides #### Cons From e26e9f76adf73a456d86433bbd49cf8a718d8b9c Mon Sep 17 00:00:00 2001 From: Lingfan Gao Date: Thu, 15 Sep 2022 09:48:36 +0200 Subject: [PATCH 6/7] update code sample --- .../convergence/reusing-avatar-in-components.md | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/rfcs/react-components/convergence/reusing-avatar-in-components.md b/rfcs/react-components/convergence/reusing-avatar-in-components.md index fa2379325f9306..d0069bbe1fd09e 100644 --- a/rfcs/react-components/convergence/reusing-avatar-in-components.md +++ b/rfcs/react-components/convergence/reusing-avatar-in-components.md @@ -229,16 +229,18 @@ const tableAvatarSizeMap = { smaller: 20, }; -export function renderTable(state) { - const { size } = state; - const { slots, slotProps } = getSlots(state); +export const renderTableCellLayout_unstable = state => { + const { slots, slotProps } = getSlots(state); return ( - {slotProps.root.children} + {/* Only affects the specific slot */} + + {slots.media && } + ); -} +}; ``` #### Pros @@ -250,7 +252,7 @@ export function renderTable(state) { - Extra code in base component - Needs prototyping and investigation -- All avatars under this context will be affected +- If the slot is not speicfic enough -> all avatars under this context will be affected - This might not scale - AvatarContext, IconContext... ## Discarded Solutions From 2feea251f9dfb5481d028bef2f617247310ec316 Mon Sep 17 00:00:00 2001 From: Lingfan Gao Date: Thu, 29 Sep 2022 17:57:04 +0200 Subject: [PATCH 7/7] update preferred solution --- .../reusing-avatar-in-components.md | 89 ++++++++++--------- 1 file changed, 46 insertions(+), 43 deletions(-) diff --git a/rfcs/react-components/convergence/reusing-avatar-in-components.md b/rfcs/react-components/convergence/reusing-avatar-in-components.md index 1f3b06912e88da..25f5f74a603fba 100644 --- a/rfcs/react-components/convergence/reusing-avatar-in-components.md +++ b/rfcs/react-components/convergence/reusing-avatar-in-components.md @@ -43,6 +43,48 @@ to do some extra work to make sure that the avatar follows design guidance. ## Detailed Design or Proposal +### Shared context + +We could consider a `AvatarContext` that is a part of `react-shared-contexts` that can be used by components that wish +to reuse the `Avatar`. + +This solution is prototyped in [#24807](https://github.com/microsoft/fluentui/pull/24807) + +```tsx +const tableAvatarSizeMap = { + small: 24, + smaller: 20, +}; + +export const renderTableCellLayout_unstable = state => { + const { slots, slotProps } = getSlots(state); + + return ( + + {/* Only affects the specific slot */} + + {slots.media && } + + + ); +}; +``` + +#### Pros + +- We also use context for lots of other user cases - it works +- Can be targeted to the speific slot that needs these overrides +- Cheap cost of context ([#24991](https://github.com/microsoft/fluentui/pull/24991) measures perf + +#### Cons + +- Extra code in base component +- Needs prototyping and investigation +- If the slot is not speicfic enough -> all avatars under this context will be affected +- This might not scale - AvatarContext, IconContext... + +## Discarded Solutions + ### Userland problem We could treat this as a userland problem, and require that apps/features follow design guidance when using an @@ -115,6 +157,10 @@ const useStyles = makeStyles({ - Does not handle props - Harder for users to override styles since selectors are more specific - Not obvious for users how base component adjusts to the parent component - magic +- Scope is not necessarily certain, in the case of avatar there are many factors that affect overrides + - font size + - icon size + - extra overrides for the badge ### Recompose components @@ -217,49 +263,6 @@ export function renderTableCellLayout() { - Bundle size - users who don't want to use avatar will have it in their bundle - Confusing priorities - One slot wins over the other, it's why we have `appearance` prop -### Shared context - -We could consider a `AvatarContext` that is a part of `react-shared-contexts` that can be used by components that wish -to reuse the `Avatar`. - -This solution is prototyped in [#24807](https://github.com/microsoft/fluentui/pull/24807) - -```tsx -const tableAvatarSizeMap = { - small: 24, - smaller: 20, -}; - -export const renderTableCellLayout_unstable = state => { - const { slots, slotProps } = getSlots(state); - - return ( - - {/* Only affects the specific slot */} - - {slots.media && } - - - ); -}; -``` - -#### Pros - -- We also use context for lots of other user cases - it works -- Can be targeted to the speific slot that needs these overrides - -#### Cons - -- Extra code in base component -- Needs prototyping and investigation -- If the slot is not speicfic enough -> all avatars under this context will be affected -- This might not scale - AvatarContext, IconContext... - -## Discarded Solutions - -> TODO put discarded solutions here - ## Open Issues