react-tabster - Add useFocusableGroup hook#20131
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 cc4e8b7:
|
📊 Bundle size report
Unchanged fixtures
|
Asset size changesSize Auditor did not detect a change in bundle size for any component! Baseline commit: 3236d4ee24994608df31c621d77e3c6943811867 (build) |
Perf Analysis (
|
| Scenario | Render type | Master Ticks | PR Ticks | Iterations | Status |
|---|---|---|---|---|---|
| Avatar | mount | 987 | 966 | 5000 | |
| BaseButton | mount | 948 | 918 | 5000 | |
| Breadcrumb | mount | 2686 | 2673 | 1000 | |
| ButtonNext | mount | 525 | 530 | 5000 | |
| Checkbox | mount | 1593 | 1606 | 5000 | |
| CheckboxBase | mount | 1344 | 1307 | 5000 | |
| ChoiceGroup | mount | 4872 | 4778 | 5000 | |
| ComboBox | mount | 1035 | 1091 | 1000 | |
| CommandBar | mount | 10440 | 10471 | 1000 | |
| ContextualMenu | mount | 6786 | 6662 | 1000 | |
| DefaultButton | mount | 1198 | 1183 | 5000 | |
| DetailsRow | mount | 3809 | 3886 | 5000 | |
| DetailsRowFast | mount | 3868 | 3840 | 5000 | |
| DetailsRowNoStyles | mount | 3671 | 3722 | 5000 | |
| Dialog | mount | 2619 | 2699 | 1000 | |
| DocumentCardTitle | mount | 185 | 175 | 1000 | |
| Dropdown | mount | 3320 | 3323 | 5000 | |
| FluentProviderNext | mount | 3472 | 3496 | 5000 | |
| FluentProviderWithTheme | mount | 217 | 216 | 10 | |
| FluentProviderWithTheme | virtual-rerender | 110 | 106 | 10 | |
| FluentProviderWithTheme | virtual-rerender-with-unmount | 265 | 268 | 10 | |
| FocusTrapZone | mount | 1883 | 1845 | 5000 | |
| FocusZone | mount | 1974 | 1805 | 5000 | |
| IconButton | mount | 1834 | 1788 | 5000 | |
| Label | mount | 359 | 364 | 5000 | |
| Layer | mount | 3058 | 3063 | 5000 | |
| Link | mount | 513 | 494 | 5000 | |
| MakeStyles | mount | 1841 | 1883 | 50000 | |
| MenuButton | mount | 1503 | 1510 | 5000 | |
| MessageBar | mount | 2106 | 2052 | 5000 | |
| Nav | mount | 3321 | 3339 | 1000 | |
| OverflowSet | mount | 1130 | 1158 | 5000 | |
| Panel | mount | 2558 | 2537 | 1000 | |
| Persona | mount | 865 | 886 | 1000 | |
| Pivot | mount | 1449 | 1452 | 1000 | |
| PrimaryButton | mount | 1295 | 1349 | 5000 | |
| Rating | mount | 8015 | 7718 | 5000 | |
| SearchBox | mount | 1354 | 1369 | 5000 | |
| Shimmer | mount | 2565 | 2565 | 5000 | |
| Slider | mount | 1992 | 2075 | 5000 | |
| SpinButton | mount | 5104 | 5029 | 5000 | |
| Spinner | mount | 441 | 467 | 5000 | |
| SplitButton | mount | 3272 | 3157 | 5000 | |
| Stack | mount | 538 | 524 | 5000 | |
| StackWithIntrinsicChildren | mount | 1772 | 1777 | 5000 | |
| StackWithTextChildren | mount | 4811 | 4713 | 5000 | |
| SwatchColorPicker | mount | 10629 | 10740 | 5000 | |
| Tabs | mount | 1470 | 1502 | 1000 | |
| TagPicker | mount | 2652 | 2628 | 5000 | |
| TeachingBubble | mount | 13372 | 13402 | 5000 | |
| Text | mount | 462 | 448 | 5000 | |
| TextField | mount | 1395 | 1454 | 5000 | |
| ThemeProvider | mount | 1294 | 1298 | 5000 | |
| ThemeProvider | virtual-rerender | 635 | 629 | 5000 | |
| ThemeProvider | virtual-rerender-with-unmount | 1938 | 1955 | 5000 | |
| Toggle | mount | 905 | 868 | 5000 | |
| buttonNative | mount | 158 | 147 | 5000 |
Perf Analysis (@fluentui/react-northstar)
Perf tests with no regressions
| Scenario | Current PR Ticks | Baseline Ticks | Ratio |
|---|---|---|---|
| PortalMinimalPerf.default | 199 | 180 | 1.11:1 |
| BoxMinimalPerf.default | 379 | 344 | 1.1:1 |
| ChatWithPopoverPerf.default | 407 | 374 | 1.09:1 |
| LabelMinimalPerf.default | 410 | 377 | 1.09:1 |
| AttachmentMinimalPerf.default | 174 | 161 | 1.08:1 |
| ChatDuplicateMessagesPerf.default | 322 | 299 | 1.08:1 |
| AvatarMinimalPerf.default | 205 | 194 | 1.06:1 |
| ListNestedPerf.default | 580 | 547 | 1.06:1 |
| TooltipMinimalPerf.default | 1069 | 1013 | 1.06:1 |
| ButtonMinimalPerf.default | 179 | 170 | 1.05:1 |
| ButtonSlotsPerf.default | 572 | 545 | 1.05:1 |
| CardMinimalPerf.default | 577 | 550 | 1.05:1 |
| DropdownManyItemsPerf.default | 696 | 666 | 1.05:1 |
| InputMinimalPerf.default | 1366 | 1306 | 1.05:1 |
| LayoutMinimalPerf.default | 374 | 361 | 1.04:1 |
| ListWith60ListItems.default | 667 | 641 | 1.04:1 |
| ProviderMinimalPerf.default | 1174 | 1128 | 1.04:1 |
| AttachmentSlotsPerf.default | 1106 | 1072 | 1.03:1 |
| FormMinimalPerf.default | 428 | 416 | 1.03:1 |
| MenuMinimalPerf.default | 872 | 848 | 1.03:1 |
| SegmentMinimalPerf.default | 354 | 344 | 1.03:1 |
| SkeletonMinimalPerf.default | 360 | 348 | 1.03:1 |
| TreeMinimalPerf.default | 818 | 795 | 1.03:1 |
| CheckboxMinimalPerf.default | 2813 | 2764 | 1.02:1 |
| DividerMinimalPerf.default | 368 | 361 | 1.02:1 |
| TableMinimalPerf.default | 428 | 419 | 1.02:1 |
| TextAreaMinimalPerf.default | 528 | 519 | 1.02:1 |
| ButtonOverridesMissPerf.default | 1835 | 1820 | 1.01:1 |
| DatepickerMinimalPerf.default | 5743 | 5671 | 1.01:1 |
| DialogMinimalPerf.default | 778 | 773 | 1.01:1 |
| EmbedMinimalPerf.default | 4306 | 4268 | 1.01:1 |
| ListMinimalPerf.default | 517 | 512 | 1.01:1 |
| MenuButtonMinimalPerf.default | 1677 | 1668 | 1.01:1 |
| PopupMinimalPerf.default | 608 | 603 | 1.01:1 |
| ProviderMergeThemesPerf.default | 1769 | 1760 | 1.01:1 |
| DropdownMinimalPerf.default | 3292 | 3296 | 1:1 |
| HeaderMinimalPerf.default | 385 | 384 | 1:1 |
| ItemLayoutMinimalPerf.default | 1237 | 1237 | 1:1 |
| ListCommonPerf.default | 651 | 651 | 1:1 |
| RosterPerf.default | 1216 | 1220 | 1:1 |
| TableManyItemsPerf.default | 1949 | 1955 | 1:1 |
| CustomToolbarPrototype.default | 4234 | 4240 | 1:1 |
| AnimationMinimalPerf.default | 418 | 421 | 0.99:1 |
| ChatMinimalPerf.default | 660 | 668 | 0.99:1 |
| FlexMinimalPerf.default | 281 | 283 | 0.99:1 |
| HeaderSlotsPerf.default | 758 | 767 | 0.99:1 |
| RefMinimalPerf.default | 246 | 249 | 0.99:1 |
| StatusMinimalPerf.default | 673 | 683 | 0.99:1 |
| TextMinimalPerf.default | 351 | 354 | 0.99:1 |
| VideoMinimalPerf.default | 607 | 613 | 0.99:1 |
| CarouselMinimalPerf.default | 471 | 481 | 0.98:1 |
| ImageMinimalPerf.default | 379 | 386 | 0.98:1 |
| LoaderMinimalPerf.default | 702 | 715 | 0.98:1 |
| SplitButtonMinimalPerf.default | 4267 | 4348 | 0.98:1 |
| AlertMinimalPerf.default | 272 | 281 | 0.97:1 |
| GridMinimalPerf.default | 352 | 363 | 0.97:1 |
| ReactionMinimalPerf.default | 371 | 382 | 0.97:1 |
| ToolbarMinimalPerf.default | 956 | 986 | 0.97:1 |
| TreeWith60ListItems.default | 183 | 188 | 0.97:1 |
| AccordionMinimalPerf.default | 155 | 163 | 0.95:1 |
| SliderMinimalPerf.default | 1717 | 1799 | 0.95:1 |
| RadioGroupMinimalPerf.default | 459 | 488 | 0.94:1 |
| IconMinimalPerf.default | 590 | 640 | 0.92:1 |
|
N* issues seem to be unrelated as this is a new hook |
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
|
Maybe a bit late on this, but I think the spelling should be "grouper" not "groupper" (would need to be fixed in tabster as well, but it's probably worth doing in the interest of having proper spelling in public APIs) |
@ecraig12345 here is a grouper 😁 |
| * A hook that returns the necessary tabster attributes to support groupping. | ||
| * @param options - Options to configure keyboard navigation | ||
| */ | ||
| export const useGroupper = (options?: UseGroupperOptions) => { |
There was a problem hiding this comment.
I would advocate not mapping the names 1:1, the tabster naming kind of assumes you have some background into focus management.
something like useFocusableGroup or smth like that could work
There was a problem hiding this comment.
I totally agree, went with it for lack of a better name.
I'll change it to useFocusableGroup.
There was a problem hiding this comment.
Also changing the enum to FocusableGroupTabBehavior to be more descriptive. Not a big fan of the big name but this way we can prevent possible collisions.
| import { useTabsterAttributes } from './useTabsterAttributes'; | ||
| import { useTabster } from './useTabster'; | ||
|
|
||
| export enum FocusableGroupTabBehavior { |
There was a problem hiding this comment.
Do we need to redefine this enum? Couldn't we simply reuse the type already provided by Tabster?
There was a problem hiding this comment.
We could, but needing to import tabster separately just for the types seemed like a bad DX for me. Plus, we can give it a more user friendly name.
Are there any downsides to this?
There was a problem hiding this comment.
I would say let's get rid of enums , they are not consistent for the API surface of our library that generally uses string values
take a look at useArrowNagivationGroup and how string props are mapped to the enums
There was a problem hiding this comment.
@andrefcdias sorry, I did not construct my intent clearly enough. My idea was to re-export the type from Tabster, instead of redefining it. That way it's still accessible directly from react-tabster package, but in case Tabster decides to add another option to the enum we don't have to go and update types in this package as they are still passed in directly.
There was a problem hiding this comment.
Enums are also compiled into non tree shakable code, TypeScript playground
export var A;
(function (A) {
A["Foo"] = "foo";
A["Bar"] = "baz";
})(A || (A = {}));There was a problem hiding this comment.
I'll go with the string option then to match the API surface, like useArrowNavigationGroup
I know, but nearly the only reference to "groupper" on the web is in tabster. |
Description of changes
Added new hook to
react-tabsterfor the tabster groupper behavior.