Skip to content

react-tabster - Add useFocusableGroup hook#20131

Merged
theerebuss merged 11 commits into
microsoft:masterfrom
theerebuss:add-tabster-groupper-hook
Nov 5, 2021
Merged

react-tabster - Add useFocusableGroup hook#20131
theerebuss merged 11 commits into
microsoft:masterfrom
theerebuss:add-tabster-groupper-hook

Conversation

@theerebuss

@theerebuss theerebuss commented Oct 6, 2021

Copy link
Copy Markdown
Contributor

Description of changes

Added new hook to react-tabster for the tabster groupper behavior.

@codesandbox-ci

codesandbox-ci Bot commented Oct 6, 2021

Copy link
Copy Markdown

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:

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

@fabricteam

fabricteam commented Oct 6, 2021

Copy link
Copy Markdown
Collaborator

📊 Bundle size report

Package & Exports Baseline (minified/GZIP) PR Change
react-card
Card - All
49.008 kB
14.564 kB
49.002 kB
14.554 kB
-6 B
-10 B
react-card
Card
44.495 kB
13.343 kB
44.489 kB
13.335 kB
-6 B
-8 B
Unchanged fixtures
Package & Exports Size (minified/GZIP)
react-accordion
Accordion (including children components)
57.707 kB
18.166 kB
react-button
Button
25.514 kB
7.744 kB
react-button
CompoundButton
30.771 kB
8.696 kB
react-button
MenuButton
27.539 kB
8.42 kB
react-button
SplitButton
33.65 kB
9.603 kB
react-button
ToggleButton
34.74 kB
8.396 kB
react-card
CardFooter
8.141 kB
3.43 kB
react-card
CardHeader
9.461 kB
3.883 kB
react-card
CardPreview
8.434 kB
3.604 kB
react-components
react-components: Accordion, Button, FluentProvider, Image, Menu, Popover
169.348 kB
48.062 kB
react-components
react-components: FluentProvider & webLightTheme
32.201 kB
10.665 kB
react-link
Link
11.659 kB
4.704 kB
react-menu
Menu (including children components)
105.802 kB
32.207 kB
react-menu
Menu (including selectable components)
108.078 kB
32.58 kB
react-popover
Popover
101.166 kB
30.376 kB
react-portal
Portal
6.725 kB
2.237 kB
react-provider
FluentProvider
15.16 kB
5.58 kB
react-slider
RangedSlider
41.432 kB
11.976 kB
react-slider
Slider
34.81 kB
10.862 kB
react-switch
Switch
26.615 kB
8.562 kB
react-tooltip
Tooltip
45.556 kB
15.549 kB
🤖 This report was generated against 3236d4ee24994608df31c621d77e3c6943811867

@size-auditor

size-auditor Bot commented Oct 6, 2021

Copy link
Copy Markdown

Asset size changes

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

Baseline commit: 3236d4ee24994608df31c621d77e3c6943811867 (build)

@fabricteam

fabricteam commented Oct 6, 2021

Copy link
Copy Markdown
Collaborator

Perf Analysis (@fluentui/react)

No significant results to display.

All results

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

@theerebuss theerebuss marked this pull request as ready for review October 6, 2021 18:54
@theerebuss theerebuss requested a review from a team as a code owner October 6, 2021 18:54
@theerebuss

Copy link
Copy Markdown
Contributor Author

N* issues seem to be unrelated as this is a new hook

@theerebuss theerebuss requested review from a team October 6, 2021 18:55
@theerebuss theerebuss mentioned this pull request Oct 6, 2021
1 task
@theerebuss

Copy link
Copy Markdown
Contributor Author

/azp run

@theerebuss theerebuss closed this Oct 6, 2021
@theerebuss theerebuss reopened this Oct 6, 2021
@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 3 pipeline(s).

@ecraig12345

ecraig12345 commented Oct 7, 2021

Copy link
Copy Markdown
Member

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)

@theerebuss theerebuss requested a review from ling1726 October 7, 2021 07:16
@ling1726

ling1726 commented Oct 7, 2021

Copy link
Copy Markdown
Contributor

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) => {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I totally agree, went with it for lack of a better name.
I'll change it to useFocusableGroup.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@theerebuss theerebuss changed the title react-tabster - Add useGroupper hook react-tabster - Add useFocusableGroup hook Oct 7, 2021
@theerebuss theerebuss requested a review from ling1726 October 7, 2021 11:16
@varholak-peter varholak-peter self-assigned this Oct 7, 2021
import { useTabsterAttributes } from './useTabsterAttributes';
import { useTabster } from './useTabster';

export enum FocusableGroupTabBehavior {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to redefine this enum? Couldn't we simply reuse the type already provided by Tabster?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

@ling1726 ling1726 Oct 7, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

@layershifter layershifter Oct 7, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Enums are also compiled into non tree shakable code, TypeScript playground

export var A;
(function (A) {
    A["Foo"] = "foo";
    A["Bar"] = "baz";
})(A || (A = {}));

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll go with the string option then to match the API surface, like useArrowNavigationGroup

@varholak-peter varholak-peter left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🇳🇮

@ecraig12345

Copy link
Copy Markdown
Member

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 😁

I know, but nearly the only reference to "groupper" on the web is in tabster.

@theerebuss theerebuss merged commit 70e81a5 into microsoft:master Nov 5, 2021
mlp73 pushed a commit to mlp73/fluentui that referenced this pull request Jan 17, 2022
@theerebuss theerebuss deleted the add-tabster-groupper-hook branch May 12, 2022 07:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants