feat: Implement shared AvatarContext by ling1726 · Pull Request #24807 · microsoft/fluentui

ling1726 · 2022-09-15T07:38:47Z

Done as part or RFC 24790. Creates a new context in react-shared-contexts that will be consumed by the Avatar component.

This context provider can be used only internally and it is up to the component author to make sure that the provider is located as close as possible to where the overrides need to occur.

The PR includes the first usage of this context in the media slot of TableCellLayout

Fixes #24805

Done as part or RFC 24790. Creates a new context in react-shared-contexts that will be consumed by the `Avatar` component. This context provider can be used only internally and it is up to the component author to make sure that the provider is located as close as possible to where the overrides need to occur.

fabricteam · 2022-09-15T07:50:58Z

📊 Bundle size report

Package & Exports	Baseline (minified/GZIP)	PR	Change
`react-alert` Alert	`83.822 kB` `21.029 kB`	`83.958 kB` `21.073 kB`	`136 B` `44 B`
`react-avatar` Avatar	`48.692 kB` `13.8 kB`	`48.828 kB` `13.842 kB`	`136 B` `42 B`
`react-avatar` AvatarGroupItem	`68.66 kB` `19.138 kB`	`68.796 kB` `19.182 kB`	`136 B` `44 B`

Unchanged fixtures

Package & Exports	Size (minified/GZIP)
`react-avatar` AvatarGroup	`14.95 kB` `5.989 kB`
`react-components` react-components: Accordion, Button, FluentProvider, Image, Menu, Popover	`188.681 kB` `52.366 kB`
`react-components` react-components: FluentProvider & webLightTheme	`33.394 kB` `11.007 kB`
`react-portal-compat` PortalCompatProvider	`5.851 kB` `1.964 kB`

_{🤖 This report was generated against d7c027ced7d54b83d6cc2897350a6adb856dcfef}

size-auditor · 2022-09-15T07:53:06Z

Asset size changes

⚠️ Insufficient baseline data to detect size changes

Unable to find bundle size details for Baseline commit: a0d258e

Possible causes

The baseline build a0d258e is broken
The Size Auditor run for the baseline build a0d258e was not triggered

Recommendations

Please merge your branch for this Pull request with the latest master build and commit your changes once again

…context

codesandbox-ci · 2022-09-15T08:27:19Z

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 810164e:

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

fabricteam · 2022-09-15T08:31:15Z

Perf Analysis (`@fluentui/react-components`)

No significant results to display.

All results

Scenario	Render type	Master Ticks	PR Ticks	Iterations
Avatar	mount	1689	1697	5000
Button	mount	1178	1179	5000
FluentProvider	mount	1959	1941	5000
FluentProviderWithTheme	mount	720	717	10
FluentProviderWithTheme	virtual-rerender	667	679	10
FluentProviderWithTheme	virtual-rerender-with-unmount	730	734	10
MakeStyles	mount	2338	2311	50000
SpinButton	mount	3305	3237	5000

layershifter · 2022-09-16T11:07:42Z

packages/react-components/react-shared-contexts/src/AvatarContext.ts

@@ -0,0 +1,28 @@
+import * as React from 'react';


It's up to you, but I think that it should be hosted in a separate package as portal-compat-context.

Or is there any real issue with including it in the react-avatar package? The rest of react-avatar should be tree shaken out of react-table if it's not used, right?

I don't mind, I've moved the avatar context to react-avatar in e29a351

layershifter · 2022-09-16T11:09:42Z

...act-components/react-table/src/components/TableCellLayout/useTableCellLayoutContextValues.ts

+
+  const avatar = React.useMemo(
+    () => ({
+      size: tableAvatarSizeMap[tableSize],


IMO, a value should be stored in state so it could be overridden with composition patterns.

you're right, I added avatarSize to state

ling1726 · 2022-09-26T19:36:34Z

Could also be moved to a specific package like @fluentui/react-avatar-context WDYT @layershifter @behowell

…context

behowell · 2022-09-27T17:54:44Z

packages/react-components/react-avatar/src/components/Avatar/Avatar.types.ts

@@ -1,5 +1,6 @@
 import { PresenceBadge } from '@fluentui/react-badge';
 import type { ComponentProps, ComponentState, Slot } from '@fluentui/react-utilities';
+import type { AvatarSizes as AvatarContextSizes } from '@fluentui/react-shared-contexts';


Would it work to just export it directly?

Suggested change

import type { AvatarSizes as AvatarContextSizes } from '@fluentui/react-shared-contexts';

export type { AvatarSizes } from '@fluentui/react-shared-contexts';

yeah that would work

Fixed in e48ff9c

behowell · 2022-09-27T20:32:21Z

packages/react-components/react-table/src/components/TableCellLayout/renderTableCellLayout.tsx

+      <AvatarContextProvider value={contextValues.avatar}>
+        {slots.media && <slots.media {...slotProps.media} />}
+      </AvatarContextProvider>


Don't need to include the AvatarContextProvider if there's no media content:

Suggested change

<AvatarContextProvider value={contextValues.avatar}>

{slots.media && <slots.media {...slotProps.media} />}

</AvatarContextProvider>

{slots.media && (

<AvatarContextProvider value={contextValues.avatar}>

<slots.media {...slotProps.media} />

</AvatarContextProvider>

)}

Fixed in e29a351

behowell · 2022-09-27T20:34:04Z

packages/react-components/react-table/src/components/TableCellLayout/useTableCellLayout.ts

+import { useTableContext } from '../../contexts/tableContext';
+
+const tableAvatarSizeMap = {
+  medium: undefined,


Should this be 32 instead of undefined? Unless table specifically doesn't care about the size in this state.

Suggested change

medium: undefined,

medium: 32,

32 is the default so I didn't think I'd need to add it, but I can

Added in f792ca2

behowell · 2022-09-27T20:49:16Z

packages/react-components/react-table/src/stories/Table/SizeSmall.stories.tsx

-                    badge={{ status: item.author.status as PresenceBadgeStatus }}
-                    size={24}
-                  />
+                  <Avatar name={item.author.label} badge={{ status: item.author.status as PresenceBadgeStatus }} />


(nit) This is unrelated to this change, but we should encourage as casts like this in stories. You could either define the entire items array as const, or possibly adding making the individual statuses as const in the object, like status: 'available' as const

Fixed in bcf7dab

Thanks! I just realized I left out "not" in "should not encourage" 🤦‍♂️ but I think you figured it out 🙂

behowell · 2022-09-27T20:52:13Z

packages/react-components/react-shared-contexts/src/AvatarContext.ts

@@ -0,0 +1,28 @@
+import * as React from 'react';


Or is there any real issue with including it in the react-avatar package? The rest of react-avatar should be tree shaken out of react-table if it's not used, right?

behowell

Looks good, thanks for adding this!

behowell · 2022-09-28T17:30:17Z

packages/react-components/react-avatar/src/contexts/AvatarContext.ts

+/**
+ * Sizes for the avatar
+ */
+export type AvatarSizes = 16 | 20 | 24 | 28 | 32 | 36 | 40 | 48 | 56 | 64 | 72 | 96 | 120 | 128;


Since this is now in react-avatar, could you leave the AvatarSizes type in Avatar.types.ts, and import it here instead?

done in d0a098d

behowell · 2022-09-28T17:40:32Z

change/@fluentui-react-shared-contexts-cb55fa3a-95bc-492f-aa57-1a7616941102.json

+{
+  "type": "minor",
+  "comment": "feat: Implement AvatarContext",
+  "packageName": "@fluentui/react-shared-contexts",
+  "email": "lingfangao@hotmail.com",
+  "dependentChangeType": "patch"
+}


This can be removed

Removed in ef70cc8

behowell · 2022-09-28T17:41:29Z

change/@fluentui-react-avatar-8bdbaa84-a19a-41b4-8e84-12aaaf55bb24.json

@@ -0,0 +1,7 @@
+{
+  "type": "minor",
+  "comment": "feat: consume shared AvatarContext",


(nit) Update this description now that AvatarContext is implemented in react-avatar

Updated in ef70cc8

behowell · 2022-09-28T17:45:04Z

packages/react-components/react-table/src/components/TableCellLayout/TableCellLayout.types.ts

 */
-export type TableCellLayoutState = ComponentState<TableCellLayoutSlots> & Pick<TableCellLayoutProps, 'appearance'>;
+export type TableCellLayoutState = ComponentState<TableCellLayoutSlots> &
+  Pick<TableCellLayoutProps, 'appearance'> & { size: TableContextValue['size']; avatarSize: AvatarSizes | undefined };


(nit) You don't seem to be using the new size state variable anywhere (unless I'm missing it). I suppose it doesn't hurt to have it, but is it needed?

done in 810164e

…context

* feat: Implement shared AvatarContext Done as part or RFC 24790. Creates a new context in react-shared-contexts that will be consumed by the `Avatar` component. This context provider can be used only internally and it is up to the component author to make sure that the provider is located as close as possible to where the overrides need to occur. * update md * changefiles * improve types * avatar sizes source of truth * remove doc * update mdn * use react-avatar-context * avatar context belongs in react-avatar * be explicit about medium size * avatar sizes exported from react-avatar * remove casts for status * update md * update import * define avatar size in avatar types * update changefiles * update table types

msft-fluent-ui-bot assigned Hotell Sep 15, 2022

msft-fluent-ui-bot added the Component: Table label Sep 15, 2022

github-actions bot added this to the July Project Cycle Q3 2022 milestone Sep 15, 2022

ling1726 added 2 commits September 15, 2022 09:39

update md

99a1e14

changefiles

b58bce6

Merge remote-tracking branch 'origin/master' into feat/avatar-shared-…

7d684fe

…context

bsunderhus mentioned this pull request Sep 15, 2022

RFC: Reusing Avatar in other components #24790

Merged

improve types

fa8dcb7

layershifter reviewed Sep 16, 2022

View reviewed changes

ling1726 added 3 commits September 20, 2022 14:51

avatar sizes source of truth

a1f6c92

remove doc

08b8920

update mdn

9fd4f3c

github-actions bot modified the milestones: July Project Cycle Q3 2022, September Project Cycle Q1 2022 Sep 20, 2022

conflict

c7a14f5

ling1726 marked this pull request as ready for review September 26, 2022 19:35

ling1726 requested review from a team, behowell, khmakoto and sopranopillow as code owners September 26, 2022 19:35

ling1726 added 2 commits September 27, 2022 17:41

Merge remote-tracking branch 'origin/master' into feat/avatar-shared-…

3bfafde

…context

use react-avatar-context

18ba70d

behowell reviewed Sep 27, 2022

View reviewed changes

ling1726 added 3 commits September 28, 2022 15:25

avatar context belongs in react-avatar

e29a351

be explicit about medium size

f792ca2

avatar sizes exported from react-avatar

e48ff9c

ling1726 requested a review from behowell September 28, 2022 13:32

ling1726 added 3 commits September 28, 2022 15:34

remove casts for status

bcf7dab

update md

e211073

update import

a847489

behowell approved these changes Sep 28, 2022

View reviewed changes

ling1726 added 4 commits September 28, 2022 20:01

Merge remote-tracking branch 'origin/master' into feat/avatar-shared-…

46153f1

…context

define avatar size in avatar types

d0a098d

update changefiles

ef70cc8

update table types

810164e

layershifter approved these changes Sep 29, 2022

View reviewed changes

ling1726 merged commit 9357de4 into microsoft:master Sep 29, 2022

	import type { AvatarSizes as AvatarContextSizes } from '@fluentui/react-shared-contexts';
	export type { AvatarSizes } from '@fluentui/react-shared-contexts';

Conversation

ling1726 commented Sep 15, 2022

Uh oh!

fabricteam commented Sep 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

📊 Bundle size report

Uh oh!

size-auditor bot commented Sep 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Asset size changes

Uh oh!

codesandbox-ci bot commented Sep 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fabricteam commented Sep 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ling1726 commented Sep 26, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

behowell left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

fabricteam commented Sep 15, 2022 •

edited

Loading

size-auditor bot commented Sep 15, 2022 •

edited

Loading

codesandbox-ci bot commented Sep 15, 2022 •

edited

Loading

fabricteam commented Sep 15, 2022 •

edited

Loading