Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(MenuList): Removing ignoreKeydown from MenuList. #31393

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

mshoho
Copy link
Member

@mshoho mshoho commented May 16, 2024

MenuList had ignoreKeydown for Tab to prevent Tabster from handling the Tab press. As a side effect this ignoreKeydown was affecting submenus too. This PR uses TabsterMoveFocusEvent to prevent Tabster from handling the Tab press in a particular case that doesn't affect submenus.

@mshoho mshoho requested a review from a team as a code owner May 16, 2024 14:02
@fabricteam
Copy link
Collaborator

fabricteam commented May 16, 2024

📊 Bundle size report

Package & Exports Baseline (minified/GZIP) PR Change
react-components
react-components: Accordion, Button, FluentProvider, Image, Menu, Popover
222.188 kB
62.744 kB
223.919 kB
63.168 kB
1.731 kB
424 B
react-components
react-components: entire library
1.166 MB
280.364 kB
1.166 MB
280.433 kB
284 B
69 B
react-menu
Menu (including children components)
154.746 kB
46.327 kB
156.637 kB
46.772 kB
1.891 kB
445 B
react-menu
Menu (including selectable components)
157.432 kB
46.888 kB
159.323 kB
47.334 kB
1.891 kB
446 B
react-table
DataGrid
169.681 kB
47.067 kB
169.735 kB
47.076 kB
54 B
9 B
Unchanged fixtures
Package & Exports Size (minified/GZIP)
react-accordion
Accordion (including children components)
101.791 kB
30.819 kB
react-alert
Alert
84.099 kB
23.645 kB
react-avatar
Avatar
50.537 kB
16.11 kB
react-avatar
AvatarGroup
20.107 kB
7.973 kB
react-avatar
AvatarGroupItem
65.191 kB
20.441 kB
react-breadcrumb
@fluentui/react-breadcrumb - package
117.668 kB
32.416 kB
react-button
Button
39.917 kB
11.339 kB
react-button
CompoundButton
47.278 kB
12.834 kB
react-button
MenuButton
44.698 kB
12.729 kB
react-button
SplitButton
52.71 kB
14.313 kB
react-button
ToggleButton
56.963 kB
13.243 kB
react-calendar-compat
Calendar Compat
154.849 kB
40.324 kB
react-card
Card - All
104.845 kB
29.585 kB
react-card
Card
97.858 kB
27.837 kB
react-card
CardFooter
14.375 kB
5.796 kB
react-card
CardHeader
16.618 kB
6.555 kB
react-card
CardPreview
14.418 kB
5.931 kB
react-checkbox
Checkbox
36.506 kB
12.305 kB
react-combobox
Combobox (including child components)
106.032 kB
34.071 kB
react-combobox
Dropdown (including child components)
107.447 kB
34.045 kB
react-components
react-components: Button, FluentProvider & webLightTheme
71.955 kB
20.772 kB
react-components
react-components: FluentProvider & webLightTheme
44.442 kB
14.607 kB
react-datepicker-compat
DatePicker Compat
228.881 kB
63.703 kB
react-dialog
Dialog (including children components)
100.597 kB
30.053 kB
react-link
Link
17.466 kB
7.079 kB
react-message-bar
MessageBar (all components)
24.608 kB
9.155 kB
react-persona
Persona
57.428 kB
17.99 kB
react-popover
Popover
129.074 kB
40.438 kB
react-portal
Portal
14.563 kB
5.118 kB
react-portal-compat
PortalCompatProvider
8.39 kB
2.64 kB
react-provider
FluentProvider
24.616 kB
8.903 kB
react-radio
Radio
33.802 kB
10.485 kB
react-radio
RadioGroup
15.758 kB
6.431 kB
react-slider
Slider
40.8 kB
13.201 kB
react-swatch-picker
@fluentui/react-swatch-picker - package
110.047 kB
30.74 kB
react-switch
Switch
36.444 kB
11.48 kB
react-table
Table (Primitives only)
46.175 kB
14.35 kB
react-table
Table as DataGrid
138.765 kB
37.41 kB
react-table
Table (Selection only)
77.181 kB
20.767 kB
react-table
Table (Sort only)
75.824 kB
20.369 kB
react-tag-picker
@fluentui/react-tag-picker - package
189.079 kB
55.943 kB
react-tags
InteractionTag
15.703 kB
6.253 kB
react-tags
Tag
29.408 kB
9.593 kB
react-tags
TagGroup
82.895 kB
24.635 kB
react-timepicker-compat
TimePicker
108.051 kB
35.451 kB
react-toast
Toast (including Toaster)
99.518 kB
30.001 kB
react-tooltip
Tooltip
55.606 kB
19.444 kB
🤖 This report was generated against 9f04c218e77a5ab8bb4f5961f232a4be0462e3bc

mshoho and others added 2 commits May 16, 2024 16:08
Oops.

Co-authored-by: Oleksandr Fediashov <alexander.mcgarret@gmail.com>
…seMenuList.ts

Co-authored-by: Oleksandr Fediashov <alexander.mcgarret@gmail.com>
@fabricteam
Copy link
Collaborator

fabricteam commented May 16, 2024

Perf Analysis (@fluentui/react-components)

No significant results to display.

All results

Scenario Render type Master Ticks PR Ticks Iterations Status
Avatar mount 602 596 5000
Button mount 309 309 5000
Field mount 1146 1143 5000
FluentProvider mount 706 707 5000
FluentProviderWithTheme mount 82 80 10
FluentProviderWithTheme virtual-rerender 35 34 10
FluentProviderWithTheme virtual-rerender-with-unmount 69 67 10
MakeStyles mount 842 879 50000
Persona mount 1767 1734 5000
SpinButton mount 1385 1363 5000
SwatchPicker mount 1526 1551 5000

@mshoho mshoho changed the title Menu list remove ignore keydown fix(MenuList): Removing ignoreKeydown from MenuList. May 16, 2024
Copy link

codesandbox-ci bot commented May 16, 2024

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.

Copy link
Member

@layershifter layershifter left a comment

Choose a reason for hiding this comment

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

LGTM, but I will let @ling1726 to check it

@layershifter
Copy link
Member

image

@mshoho this looks suspicious

@mshoho
Copy link
Member Author

mshoho commented May 21, 2024

image @mshoho this looks suspicious

yes, I've checked for the cause and this is due to how events are exported from Tabster — event classes are bundled regardless of not being used. I am rearranging Tabster exports and will fix that in a subsequent PR today or tomorrow.

@layershifter
Copy link
Member

image @mshoho this looks suspicious

yes, I've checked for the cause and this is due to how events are exported from Tabster — event classes are bundled regardless of not being used. I am rearranging Tabster exports and will fix that in a subsequent PR today or tomorrow.

@mshoho let's fix before merging this PR.

@mshoho
Copy link
Member Author

mshoho commented May 21, 2024

image @mshoho this looks suspicious

yes, I've checked for the cause and this is due to how events are exported from Tabster — event classes are bundled regardless of not being used. I am rearranging Tabster exports and will fix that in a subsequent PR today or tomorrow.

@mshoho let's fix before merging this PR.

#31441 (comment) — this should do it (so far it references the new canary, I'll release 8.0.0 once it's approved in the Tabster repo).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants