feat(react-card): add `focusMode` prop #22312

theerebuss · 2022-04-04T17:32:23Z

New Behavior

Introduction of the new focusMode prop according to the behavior defined in the spec.md

Introduction of:

Implementation
E2E focus behavior testing
Storybook story

Related Issue(s)

Fixes partially #19336

codesandbox-ci · 2022-04-04T17:37:49Z

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 e6a3cd3:

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

fabricteam · 2022-04-04T17:39:35Z

📊 Bundle size report

_{🤖 This report was generated against e465622ca241f3dd0534da50823a51e7311fd9ef}

fabricteam · 2022-04-04T17:43:53Z

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

No significant results to display.

All results

Scenario	Render type	Master Ticks	PR Ticks	Iterations
Avatar	mount	922	917	5000
Button	mount	581	575	5000
FluentProvider	mount	2093	1947	5000
FluentProviderWithTheme	mount	279	257	10
FluentProviderWithTheme	virtual-rerender	250	229	10
FluentProviderWithTheme	virtual-rerender-with-unmount	303	320	10
MakeStyles	mount	1688	1714	50000

size-auditor · 2022-04-04T17:54:20Z

Asset size changes

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

Baseline commit: e465622ca241f3dd0534da50823a51e7311fd9ef (build)

packages/react-card/etc/react-card.api.md

packages/react-card/e2e/Card.e2e.tsx

packages/react-card/src/components/Card/Card.types.ts

packages/react-card/src/components/Card/useCard.ts

packages/react-card/src/components/Card/Card.types.ts

packages/react-card/src/stories/CardFocusable.stories.tsx

Co-authored-by: Sean Monahan <[email protected]>

fabricteam · 2022-04-29T14:49:19Z

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

Perf tests with no regressions

Scenario	Current PR Ticks	Baseline Ticks	Ratio
ImageMinimalPerf.default	389	361	1.08:1
ListCommonPerf.default	655	613	1.07:1
AvatarMinimalPerf.default	200	189	1.06:1
PopupMinimalPerf.default	668	632	1.06:1
PortalMinimalPerf.default	177	167	1.06:1
BoxMinimalPerf.default	342	327	1.05:1
DividerMinimalPerf.default	365	349	1.05:1
MenuMinimalPerf.default	879	840	1.05:1
SliderMinimalPerf.default	1715	1638	1.05:1
FormMinimalPerf.default	407	390	1.04:1
LayoutMinimalPerf.default	365	351	1.04:1
TableMinimalPerf.default	409	394	1.04:1
ButtonSlotsPerf.default	553	538	1.03:1
DropdownManyItemsPerf.default	697	679	1.03:1
ListMinimalPerf.default	522	508	1.03:1
ListNestedPerf.default	564	546	1.03:1
RefMinimalPerf.default	231	224	1.03:1
SkeletonMinimalPerf.default	333	324	1.03:1
CustomToolbarPrototype.default	2806	2736	1.03:1
AnimationMinimalPerf.default	556	544	1.02:1
AttachmentMinimalPerf.default	155	152	1.02:1
CardMinimalPerf.default	555	544	1.02:1
ChatMinimalPerf.default	746	729	1.02:1
EmbedMinimalPerf.default	4188	4125	1.02:1
InputMinimalPerf.default	1323	1303	1.02:1
LoaderMinimalPerf.default	706	689	1.02:1
SegmentMinimalPerf.default	328	323	1.02:1
TextAreaMinimalPerf.default	497	487	1.02:1
AttachmentSlotsPerf.default	1103	1096	1.01:1
ButtonOverridesMissPerf.default	1522	1505	1.01:1
ChatDuplicateMessagesPerf.default	289	285	1.01:1
MenuButtonMinimalPerf.default	1739	1719	1.01:1
ProviderMinimalPerf.default	400	395	1.01:1
ReactionMinimalPerf.default	361	356	1.01:1
TableManyItemsPerf.default	1954	1934	1.01:1
TreeMinimalPerf.default	818	812	1.01:1
ChatWithPopoverPerf.default	398	399	1:1
CheckboxMinimalPerf.default	2761	2758	1:1
FlexMinimalPerf.default	285	286	1:1
GridMinimalPerf.default	330	330	1:1
HeaderMinimalPerf.default	360	360	1:1
ItemLayoutMinimalPerf.default	1209	1206	1:1
LabelMinimalPerf.default	389	388	1:1
ProviderMergeThemesPerf.default	1293	1298	1:1
TextMinimalPerf.default	341	342	1:1
ToolbarMinimalPerf.default	966	963	1:1
VideoMinimalPerf.default	635	637	1:1
ButtonMinimalPerf.default	157	159	0.99:1
HeaderSlotsPerf.default	764	771	0.99:1
RosterPerf.default	1119	1134	0.99:1
StatusMinimalPerf.default	656	663	0.99:1
TooltipMinimalPerf.default	1084	1092	0.99:1
AlertMinimalPerf.default	279	286	0.98:1
DialogMinimalPerf.default	767	780	0.98:1
DropdownMinimalPerf.default	3081	3136	0.98:1
RadioGroupMinimalPerf.default	425	433	0.98:1
IconMinimalPerf.default	609	622	0.98:1
TreeWith60ListItems.default	173	176	0.98:1
CarouselMinimalPerf.default	458	470	0.97:1
DatepickerMinimalPerf.default	5960	6135	0.97:1
SplitButtonMinimalPerf.default	4289	4431	0.97:1
ListWith60ListItems.default	631	656	0.96:1
AccordionMinimalPerf.default	136	146	0.93:1

fabricteam · 2022-04-29T14:52:23Z

Perf Analysis (`@fluentui/react`)

No significant results to display.

All results

Scenario	Render type	Master Ticks	PR Ticks	Iterations
BaseButton	mount	792	794	5000
Breadcrumb	mount	2406	2378	1000
Checkbox	mount	1303	1293	5000
CheckboxBase	mount	1105	1091	5000
ChoiceGroup	mount	4112	4150	5000
ComboBox	mount	853	837	1000
CommandBar	mount	9102	9157	1000
ContextualMenu	mount	10026	10129	1000
DefaultButton	mount	986	975	5000
DetailsRow	mount	3336	3359	5000
DetailsRowFast	mount	3380	3283	5000
DetailsRowNoStyles	mount	3183	3155	5000
Dialog	mount	1943	1957	1000
DocumentCardTitle	mount	153	146	1000
Dropdown	mount	2863	2853	5000
FocusTrapZone	mount	1609	1597	5000
FocusZone	mount	1613	1578	5000
IconButton	mount	1551	1507	5000
Label	mount	314	310	5000
Layer	mount	2517	2540	5000
Link	mount	404	407	5000
MenuButton	mount	1270	1300	5000
MessageBar	mount	1851	1882	5000
Nav	mount	2871	2900	1000
OverflowSet	mount	938	939	5000
Panel	mount	1882	1872	1000
Persona	mount	865	879	1000
Pivot	mount	1237	1263	1000
PrimaryButton	mount	1119	1130	5000
Rating	mount	6663	6768	5000
SearchBox	mount	1124	1128	5000
Shimmer	mount	2156	2193	5000
Slider	mount	1666	1664	5000
SpinButton	mount	4383	4339	5000
Spinner	mount	386	366	5000
SplitButton	mount	2758	2769	5000
Stack	mount	441	449	5000
StackWithIntrinsicChildren	mount	1993	1975	5000
StackWithTextChildren	mount	4508	4519	5000
SwatchColorPicker	mount	10054	10147	5000
TagPicker	mount	2359	2330	5000
TeachingBubble	mount	84582	84445	5000
Text	mount	366	372	5000
TextField	mount	1234	1220	5000
ThemeProvider	mount	1035	1010	5000
ThemeProvider	virtual-rerender	556	536	5000
ThemeProvider	virtual-rerender-with-unmount	1627	1603	5000
Toggle	mount	684	701	5000
buttonNative	mount	109	115	5000

theerebuss · 2022-04-29T15:10:17Z

Conflict merging went a bit chaotic due to the package moving but it's done now. Please re-review the changes with the new API as suggested by @spmonahan

ling1726 · 2022-05-03T06:55:32Z

packages/react-components/react-card/e2e/Card.e2e.tsx

+      it('should not be focusable', () => {
+        mountFluent(<CardSample />);
+
+        cy.get('#before').focus();


nit: you seem to use these selectors multiple times, would be worth hoisting them to variables

The duplication is intentional for readability purposes as hardcoded values makes the test clearer (less need to hop around to understand what it is doing).

ling1726 · 2022-05-03T06:58:42Z

packages/react-components/react-card/e2e/Card.e2e.tsx

+      it('should be focusable', () => {
+        mountFluent(<CardSample focusMode="tab-exit" />);
+
+        const card = cy.get('#card');
+
+        card.should('not.be.focused');
+
+        card.focus();
+
+        card.should('be.focused');
+      });


nit: the focusable card test could be in another test suite. You could use forEach to iterate through different behaviours and only write the test once. You can take a look at Popover e2e tests where it goes through controlled and uncontrolled.

Sometimes I also think duping tests can be nice because reusable tests are always a bit less readable. So up to you if you want to do this or not

I prefer to go with verbosity over efficiency for tests. Tests should be as readable as possible overall and the less "magic" we add to them, the better.

the ancient wisdom says: "it depends" :D

Hotell · 2022-05-03T17:09:59Z

packages/react-components/react-card/etc/react-card.api.md

 // @public (undocumented)
 export type CardCommons = {
    appearance: 'filled' | 'filled-alternative' | 'outline' | 'subtle';
+    focusMode: 'off' | 'no-tab' | 'tab-exit' | 'tab-only';


packages/react-components/react-card/src/stories/CardFocusMode.stories.tsx

packages/react-components/react-card/src/components/Card/useCard.ts

packages/react-components/react-card/src/components/Card/__snapshots__/Card.test.tsx.snap

Hotell · 2022-05-03T17:55:46Z

packages/react-components/react-card/e2e/Card.e2e.tsx

+      it('should be focusable', () => {
+        mountFluent(<CardSample focusMode="tab-exit" />);
+
+        const card = cy.get('#card');
+
+        card.should('not.be.focused');
+
+        card.focus();
+
+        card.should('be.focused');
+      });


the ancient wisdom says: "it depends" :D

theerebuss added 2 commits April 2, 2022 20:49

feat: add focusable prop

b311e06

chore: add type safe approach

90ffeeb

theerebuss added Type: Feature Component: Card Fluent UI react-components (v9) labels Apr 4, 2022

theerebuss added this to the April Project Cycle Q1 2022 milestone Apr 4, 2022

theerebuss mentioned this pull request Apr 4, 2022

Card Convergence #19336

Closed

37 tasks

theerebuss added 8 commits April 5, 2022 08:10

chore: add API files

8d5cb1e

fix: add missing tabindex for focusable cards

543d0e0

feat: add focus story

1054f5a

fix: lint errors

9a5e04b

chore: update snapshots

8fe5c64

Merge branch 'master' into card-focusable

80d6c7a

chore: leverage CardSample to avoid duplication

3b2b94a

chore: add e2e focus tests

160574c

theerebuss marked this pull request as ready for review April 14, 2022 15:00

theerebuss requested a review from a team as a code owner April 14, 2022 15:00

ling1726 requested a review from a team April 19, 2022 12:37

jurokapsiar reviewed Apr 19, 2022

View reviewed changes

packages/react-card/etc/react-card.api.md Outdated Show resolved Hide resolved

ling1726 reviewed Apr 19, 2022

View reviewed changes

packages/react-card/e2e/Card.e2e.tsx Outdated Show resolved Hide resolved

ling1726 reviewed Apr 19, 2022

View reviewed changes

packages/react-card/src/components/Card/Card.types.ts Outdated Show resolved Hide resolved

ling1726 reviewed Apr 19, 2022

View reviewed changes

packages/react-card/src/components/Card/useCard.ts Outdated Show resolved Hide resolved

spmonahan reviewed Apr 19, 2022

View reviewed changes

packages/react-card/src/components/Card/Card.types.ts Outdated Show resolved Hide resolved

packages/react-card/src/components/Card/Card.types.ts Outdated Show resolved Hide resolved

packages/react-card/src/stories/CardFocusable.stories.tsx Outdated Show resolved Hide resolved

theerebuss and others added 3 commits April 26, 2022 18:44

chore: remove unused import

e90464b

Update packages/react-card/src/stories/CardFocusable.stories.tsx

bd89c2a

Co-authored-by: Sean Monahan <[email protected]>

chore: rename focusable camelCase type to kebab-case

c222356

theerebuss removed request for behowell, chpalac, ermercer and tomi-msft April 29, 2022 14:34

theerebuss added 3 commits April 29, 2022 17:00

chore: move card to react-components

d192755

Merge branch 'master' into card-focusable

be9688a

fix: remove unintended change to react-button

bc54b30

theerebuss requested review from Hotell, jurokapsiar, ling1726 and spmonahan April 29, 2022 15:08

fix: address merging issues

1efb5f4

spmonahan approved these changes May 2, 2022

View reviewed changes

ling1726 reviewed May 3, 2022

View reviewed changes

ling1726 approved these changes May 3, 2022

View reviewed changes

Hotell reviewed May 3, 2022

View reviewed changes

chore: use as const for the prop map

7427282

theerebuss requested a review from Hotell May 4, 2022 10:20

chore: remove unused imports

77b469d

theerebuss modified the milestones: April Project Cycle Q1 2022, May Project Cycle Q2 2022 May 4, 2022

Hotell approved these changes May 4, 2022

View reviewed changes

chore: change story name to match prop

e6a3cd3

theerebuss merged commit 6f5126a into microsoft:master May 5, 2022

theerebuss deleted the card-focusable branch May 5, 2022 13:45

marwan38 pushed a commit to marwan38/fluentui that referenced this pull request Jun 13, 2022

feat(react-card): add focusMode prop (microsoft#22312)

df1ba14

feat(react-card): add focusMode prop #22312

feat(react-card): add focusMode prop #22312

Uh oh!

Conversation

theerebuss commented Apr 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

New Behavior

Related Issue(s)

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

📊 Bundle size report

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Asset size changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

fabricteam commented Apr 29, 2022

Perf Analysis (@fluentui/react-northstar)

Uh oh!

fabricteam commented Apr 29, 2022

Uh oh!

theerebuss commented Apr 29, 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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

feat(react-card): add `focusMode` prop #22312

feat(react-card): add `focusMode` prop #22312

theerebuss commented Apr 4, 2022 •

edited

Loading

codesandbox-ci bot commented Apr 4, 2022 •

edited

Loading

fabricteam commented Apr 4, 2022 •

edited

Loading

fabricteam commented Apr 4, 2022 •

edited

Loading

size-auditor bot commented Apr 4, 2022 •

edited

Loading

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