feat(react-card): add focusMode prop#22312
Merged
theerebuss merged 25 commits intomicrosoft:masterfrom May 5, 2022
theerebuss:card-focusable
Merged
feat(react-card): add focusMode prop#22312theerebuss merged 25 commits intomicrosoft:masterfrom theerebuss:card-focusable
focusMode prop#22312theerebuss merged 25 commits intomicrosoft:masterfrom
theerebuss:card-focusable
Conversation
|
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:
|
Collaborator
📊 Bundle size report🤖 This report was generated against e465622ca241f3dd0534da50823a51e7311fd9ef |
Collaborator
Perf Analysis (
|
| Scenario | Render type | Master Ticks | PR Ticks | Iterations | Status |
|---|---|---|---|---|---|
| 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 |
Asset size changesSize Auditor did not detect a change in bundle size for any component! Baseline commit: e465622ca241f3dd0534da50823a51e7311fd9ef (build) |
jurokapsiar
reviewed
Apr 19, 2022
ling1726
reviewed
Apr 19, 2022
ling1726
reviewed
Apr 19, 2022
ling1726
reviewed
Apr 19, 2022
spmonahan
reviewed
Apr 19, 2022
Co-authored-by: Sean Monahan <seanmonahan@microsoft.com>
Collaborator
Perf Analysis (
|
| 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 |
Collaborator
Perf Analysis (
|
| Scenario | Render type | Master Ticks | PR Ticks | Iterations | Status |
|---|---|---|---|---|---|
| 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 |
Contributor
Author
|
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 |
spmonahan
approved these changes
May 2, 2022
ling1726
reviewed
May 3, 2022
| it('should not be focusable', () => { | ||
| mountFluent(<CardSample />); | ||
|
|
||
| cy.get('#before').focus(); |
Contributor
There was a problem hiding this comment.
nit: you seem to use these selectors multiple times, would be worth hoisting them to variables
Contributor
Author
There was a problem hiding this comment.
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
reviewed
May 3, 2022
Comment on lines
+130
to
+140
| 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'); | ||
| }); |
Contributor
There was a problem hiding this comment.
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
Contributor
Author
There was a problem hiding this comment.
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.
Contributor
There was a problem hiding this comment.
the ancient wisdom says: "it depends" :D
ling1726
approved these changes
May 3, 2022
Hotell
reviewed
May 3, 2022
| // @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
Outdated
Show resolved
Hide resolved
packages/react-components/react-card/src/components/Card/useCard.ts
Outdated
Show resolved
Hide resolved
packages/react-components/react-card/src/components/Card/__snapshots__/Card.test.tsx.snap
Show resolved
Hide resolved
Comment on lines
+130
to
+140
| 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'); | ||
| }); |
Contributor
There was a problem hiding this comment.
the ancient wisdom says: "it depends" :D
Hotell
approved these changes
May 4, 2022
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
7 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
New Behavior
Introduction of the new
focusModeprop according to the behavior defined in the spec.mdIntroduction of:
Related Issue(s)
Fixes partially #19336