react-card - Phase 2#20132
Merged
Merged
Conversation
added 3 commits
October 6, 2021 20:09
Collaborator
📊 Bundle size report
Unchanged fixtures
|
|
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 9b43855:
|
Asset size changesSize Auditor did not detect a change in bundle size for any component! Baseline commit: a8e02933f183d7336c4922bad549c0976afc93af (build) |
Collaborator
Perf Analysis (
|
| Scenario | Render type | Master Ticks | PR Ticks | Iterations | Status |
|---|---|---|---|---|---|
| Avatar | mount | 1028 | 1061 | 5000 | |
| BaseButton | mount | 992 | 1013 | 5000 | |
| Breadcrumb | mount | 2666 | 2712 | 1000 | |
| ButtonNext | mount | 538 | 531 | 5000 | |
| Checkbox | mount | 1702 | 1708 | 5000 | |
| CheckboxBase | mount | 1475 | 1429 | 5000 | |
| ChoiceGroup | mount | 5148 | 5186 | 5000 | |
| ComboBox | mount | 1023 | 1025 | 1000 | |
| CommandBar | mount | 10512 | 10403 | 1000 | |
| ContextualMenu | mount | 6586 | 6528 | 1000 | |
| DefaultButton | mount | 1247 | 1243 | 5000 | |
| DetailsRow | mount | 4072 | 4133 | 5000 | |
| DetailsRowFast | mount | 4053 | 4031 | 5000 | |
| DetailsRowNoStyles | mount | 3926 | 3891 | 5000 | |
| Dialog | mount | 2576 | 2630 | 1000 | |
| DocumentCardTitle | mount | 180 | 183 | 1000 | |
| Dropdown | mount | 3458 | 3501 | 5000 | |
| FluentProviderNext | mount | 3286 | 3247 | 5000 | |
| FluentProviderWithTheme | mount | 199 | 215 | 10 | |
| FluentProviderWithTheme | virtual-rerender | 110 | 97 | 10 | |
| FluentProviderWithTheme | virtual-rerender-with-unmount | 242 | 227 | 10 | |
| FocusTrapZone | mount | 1942 | 1918 | 5000 | |
| FocusZone | mount | 1861 | 1924 | 5000 | |
| IconButton | mount | 1903 | 1947 | 5000 | |
| Label | mount | 373 | 360 | 5000 | |
| Layer | mount | 3206 | 3234 | 5000 | |
| Link | mount | 517 | 525 | 5000 | |
| MakeStyles | mount | 1896 | 1835 | 50000 | |
| MenuButton | mount | 1702 | 1616 | 5000 | |
| MessageBar | mount | 2078 | 2091 | 5000 | |
| Nav | mount | 3530 | 3573 | 1000 | |
| OverflowSet | mount | 1167 | 1217 | 5000 | |
| Panel | mount | 2502 | 2487 | 1000 | |
| Persona | mount | 904 | 913 | 1000 | |
| Pivot | mount | 1508 | 1522 | 1000 | |
| PrimaryButton | mount | 1403 | 1386 | 5000 | |
| Rating | mount | 8537 | 8664 | 5000 | |
| SearchBox | mount | 1493 | 1477 | 5000 | |
| Shimmer | mount | 2758 | 2736 | 5000 | |
| Slider | mount | 2116 | 2116 | 5000 | |
| SpinButton | mount | 5378 | 5327 | 5000 | |
| Spinner | mount | 451 | 463 | 5000 | |
| SplitButton | mount | 3405 | 3360 | 5000 | |
| Stack | mount | 549 | 573 | 5000 | |
| StackWithIntrinsicChildren | mount | 1845 | 1959 | 5000 | |
| StackWithTextChildren | mount | 5151 | 5172 | 5000 | |
| SwatchColorPicker | mount | 11606 | 11742 | 5000 | |
| Tabs | mount | 1571 | 1574 | 1000 | |
| TagPicker | mount | 2925 | 2905 | 5000 | |
| TeachingBubble | mount | 13598 | 13735 | 5000 | |
| Text | mount | 499 | 494 | 5000 | |
| TextField | mount | 1553 | 1582 | 5000 | |
| ThemeProvider | mount | 1294 | 1297 | 5000 | |
| ThemeProvider | virtual-rerender | 675 | 688 | 5000 | |
| ThemeProvider | virtual-rerender-with-unmount | 2201 | 2102 | 5000 | |
| Toggle | mount | 965 | 930 | 5000 | |
| buttonNative | mount | 165 | 153 | 5000 |
Perf Analysis (@fluentui/react-northstar)
Perf tests with no regressions
| Scenario | Current PR Ticks | Baseline Ticks | Ratio |
|---|---|---|---|
| LabelMinimalPerf.default | 446 | 398 | 1.12:1 |
| ButtonMinimalPerf.default | 216 | 196 | 1.1:1 |
| RefMinimalPerf.default | 252 | 230 | 1.1:1 |
| SegmentMinimalPerf.default | 405 | 368 | 1.1:1 |
| SkeletonMinimalPerf.default | 409 | 371 | 1.1:1 |
| AvatarMinimalPerf.default | 238 | 220 | 1.08:1 |
| LayoutMinimalPerf.default | 421 | 391 | 1.08:1 |
| StatusMinimalPerf.default | 746 | 693 | 1.08:1 |
| AlertMinimalPerf.default | 324 | 303 | 1.07:1 |
| AttachmentMinimalPerf.default | 183 | 171 | 1.07:1 |
| CarouselMinimalPerf.default | 538 | 505 | 1.07:1 |
| ListCommonPerf.default | 708 | 660 | 1.07:1 |
| TableMinimalPerf.default | 468 | 437 | 1.07:1 |
| ChatMinimalPerf.default | 733 | 694 | 1.06:1 |
| RadioGroupMinimalPerf.default | 501 | 471 | 1.06:1 |
| DividerMinimalPerf.default | 418 | 398 | 1.05:1 |
| ListMinimalPerf.default | 573 | 548 | 1.05:1 |
| HeaderSlotsPerf.default | 861 | 829 | 1.04:1 |
| MenuMinimalPerf.default | 918 | 882 | 1.04:1 |
| ButtonSlotsPerf.default | 605 | 589 | 1.03:1 |
| DialogMinimalPerf.default | 817 | 795 | 1.03:1 |
| GridMinimalPerf.default | 373 | 363 | 1.03:1 |
| ImageMinimalPerf.default | 423 | 411 | 1.03:1 |
| CardMinimalPerf.default | 624 | 614 | 1.02:1 |
| ChatDuplicateMessagesPerf.default | 325 | 320 | 1.02:1 |
| MenuButtonMinimalPerf.default | 1764 | 1734 | 1.02:1 |
| TextMinimalPerf.default | 380 | 371 | 1.02:1 |
| TooltipMinimalPerf.default | 1140 | 1114 | 1.02:1 |
| BoxMinimalPerf.default | 384 | 379 | 1.01:1 |
| ButtonOverridesMissPerf.default | 1974 | 1958 | 1.01:1 |
| CheckboxMinimalPerf.default | 2890 | 2856 | 1.01:1 |
| FormMinimalPerf.default | 460 | 455 | 1.01:1 |
| InputMinimalPerf.default | 1389 | 1374 | 1.01:1 |
| ItemLayoutMinimalPerf.default | 1330 | 1321 | 1.01:1 |
| PortalMinimalPerf.default | 183 | 181 | 1.01:1 |
| CustomToolbarPrototype.default | 4456 | 4409 | 1.01:1 |
| ToolbarMinimalPerf.default | 1026 | 1019 | 1.01:1 |
| TreeWith60ListItems.default | 187 | 185 | 1.01:1 |
| AccordionMinimalPerf.default | 175 | 175 | 1:1 |
| AttachmentSlotsPerf.default | 1153 | 1152 | 1:1 |
| DatepickerMinimalPerf.default | 5825 | 5828 | 1:1 |
| DropdownMinimalPerf.default | 3318 | 3323 | 1:1 |
| ListNestedPerf.default | 621 | 620 | 1:1 |
| RosterPerf.default | 1346 | 1352 | 1:1 |
| PopupMinimalPerf.default | 615 | 614 | 1:1 |
| SliderMinimalPerf.default | 1784 | 1788 | 1:1 |
| SplitButtonMinimalPerf.default | 4654 | 4647 | 1:1 |
| TextAreaMinimalPerf.default | 545 | 545 | 1:1 |
| VideoMinimalPerf.default | 692 | 692 | 1:1 |
| TableManyItemsPerf.default | 2088 | 2105 | 0.99:1 |
| DropdownManyItemsPerf.default | 756 | 771 | 0.98:1 |
| EmbedMinimalPerf.default | 4533 | 4617 | 0.98:1 |
| HeaderMinimalPerf.default | 418 | 428 | 0.98:1 |
| LoaderMinimalPerf.default | 752 | 769 | 0.98:1 |
| TreeMinimalPerf.default | 867 | 891 | 0.97:1 |
| IconMinimalPerf.default | 685 | 716 | 0.96:1 |
| AnimationMinimalPerf.default | 429 | 450 | 0.95:1 |
| ListWith60ListItems.default | 679 | 713 | 0.95:1 |
| ProviderMinimalPerf.default | 1225 | 1289 | 0.95:1 |
| ReactionMinimalPerf.default | 408 | 429 | 0.95:1 |
| FlexMinimalPerf.default | 305 | 323 | 0.94:1 |
| ProviderMergeThemesPerf.default | 1735 | 1864 | 0.93:1 |
| ChatWithPopoverPerf.default | 403 | 443 | 0.91:1 |
added 13 commits
October 7, 2021 11:16
ling1726
reviewed
Oct 7, 2021
| * @param ref - reference to root HTMLElement of CardFooter | ||
| */ | ||
| export const useCardFooter = (props: CardFooterProps, ref: React.Ref<HTMLElement>): CardFooterState => { | ||
| const arrowNavigationAttrs = useArrowNavigationGroup({ |
Contributor
There was a problem hiding this comment.
where does the keyboard navigation requirement for the card footer come from ? it's surprising to see when there's not role attached
Contributor
There was a problem hiding this comment.
agree, tabbing should be used, unless it is something like role='toolbar' which would suggest the users to use arrow keys.
Contributor
Author
There was a problem hiding this comment.
Design spec covers that items should be navigable with the arrow keys while focus is trapped in the card.
Check the Interaction & Accessibility section
Contributor
There was a problem hiding this comment.
in that case design needs to be changed because you cannot suddenly use arrow key navigation in part of the UI without using appropriate role. Standard user expectation is that all controls can be reached with Tab. If suddenly some of them are reachable by arrow keys, they would be easily missed - they are effectively hidden for a user that does not have the visual indication. Role serves as an information suggesting the user that something else can be used for navigation.
yes, there might be trap on enter to simulate drill-in, but it should be on the whole card, not on footer only. But inside of the card Tab/shift+tab needs to be used to navigate, see for example https://fluentsite.z22.web.core.windows.net/0.59.0/components/card/definition?showCode=false&showRtl=false&showTransparent=false&showVariables=false#usage-focusable-children-grid
Contributor
Author
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
ling1726
approved these changes
Oct 12, 2021
ling1726
left a comment
ling1726
left a comment
Contributor
There was a problem hiding this comment.
Approving, with the removal of keyboard navigation in the footer, can be added in later
mlp73
pushed a commit
to mlp73/fluentui
that referenced
this pull request
Jan 17, 2022
* Add useGroupper * Change files * Update API * Rename hook * Update API * Rename enum * Change options prop * react-card - Migrate Card to new prop merging / root as a slot (microsoft#20111) * Add focus keyboard interactions * Update stories to have context menus * Change files * Remove context examples because Typescript hates me * Update snapshots * Use new hook name * Formatting * revert(react-card): remove tabster behavior from card footer * fix: update snapshots Co-authored-by: varholak-peter <[email protected]>
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
5 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.
Pull request checklist
$ yarn changeDescription of changes
Added the missing keyboard/focus interactions to Card, a Groupper interaction for the main Card container and an ArrowNavigationGroup for the elements in CardFooter.
Cleanup the stories to use make-styles.
Rebased useGroupper hook from #20131