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: Make Menu openOnHover prop work again #24899

Merged
merged 2 commits into from
Sep 23, 2022

Conversation

behowell
Copy link
Contributor

@behowell behowell commented Sep 21, 2022

Current Behavior

Regression from the refactoring in PR #24562, Menu no longer respects the openOnHover prop.

New Behavior

Use the openOnHover prop in Menu again. It defaults to isSubmenu but can be overridden.

Related Issue(s)

@fabricteam
Copy link
Collaborator

fabricteam commented Sep 21, 2022

📊 Bundle size report

Package & Exports Baseline (minified/GZIP) PR Change
react-components
react-components: Accordion, Button, FluentProvider, Image, Menu, Popover
188.672 kB
52.359 kB
188.689 kB
52.37 kB
17 B
11 B
react-menu
Menu (including children components)
116.572 kB
35.778 kB
116.589 kB
35.777 kB
17 B
-1 B
react-menu
Menu (including selectable components)
119.641 kB
36.297 kB
119.658 kB
36.296 kB
17 B
-1 B
Unchanged fixtures
Package & Exports Size (minified/GZIP)
react-components
react-components: FluentProvider & webLightTheme
33.394 kB
11.007 kB
react-portal-compat
PortalCompatProvider
5.851 kB
1.964 kB
🤖 This report was generated against 3f105cca41d951f161edbb90be116565311e72d1

@codesandbox-ci
Copy link

codesandbox-ci bot commented Sep 21, 2022

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 0baf43c:

Sandbox Source
@fluentui/react 8 starter Configuration
@fluentui/react-components 9 starter Configuration
wonderful-lake-fc3oi8 Issue #24841

@size-auditor
Copy link

size-auditor bot commented Sep 21, 2022

Asset size changes

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

Baseline commit: 3f105cca41d951f161edbb90be116565311e72d1 (build)

@fabricteam
Copy link
Collaborator

fabricteam commented Sep 21, 2022

Perf Analysis (@fluentui/react-components)

No significant results to display.

All results

Scenario Render type Master Ticks PR Ticks Iterations Status
Avatar mount 1305 1320 5000
Button mount 953 949 5000
FluentProvider mount 1691 1566 5000
FluentProviderWithTheme mount 643 634 10
FluentProviderWithTheme virtual-rerender 589 593 10
FluentProviderWithTheme virtual-rerender-with-unmount 636 629 10
MakeStyles mount 1897 1895 50000
SpinButton mount 2550 2575 5000

@behowell behowell marked this pull request as ready for review September 21, 2022 23:45
@behowell behowell requested a review from a team as a code owner September 21, 2022 23:45
@ling1726
Copy link
Member

Nice catch, and thx for the fix @behowell . Do you mind adding a cypress e2e test for this ? I'm quite surprised no test failed during the original refactor 😱😱

Copy link
Contributor

@bsunderhus bsunderhus left a comment

Choose a reason for hiding this comment

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

LGMT! thx for the PR @behowell, definitely didn't see that regression meanwhile doing the refactoring! Adding an e2e test case would be ideal here though!

@NotWoods
Copy link
Contributor

We caught this in Loop with a Jest JSDOM test, so that would also work (I don't know what's standard for your repo, however)

@behowell behowell requested a review from bsunderhus September 22, 2022 19:06
@behowell
Copy link
Contributor Author

@ling1726 @bsunderhus I've added a cypress e2e test.

@behowell behowell enabled auto-merge (squash) September 22, 2022 21:33
@behowell behowell merged commit f6202c4 into microsoft:master Sep 23, 2022
@behowell behowell deleted the menu/fix-openOnHover branch September 23, 2022 17:01
marcosmoura added a commit to marcosmoura/fluentui that referenced this pull request Sep 24, 2022
* master: (29 commits)
  chore(react-tooltip): update package scaffold (microsoft#24927)
  chore(react-popover): update package scaffold (microsoft#24925)
  chore(react-overflow): update package scaffold (microsoft#24926)
  chore(react-menu): update package scaffold (microsoft#24924)
  applying package updates
  chore: Bump workspace-tools to 0.27.0 (microsoft#24914)
  fix: Make Menu openOnHover prop work again (microsoft#24899)
  stress test: convert cli scripts to typescript (microsoft#24915)
  update package manifest to only include v8 controls (microsoft#24839)
  Stress Test: add random tree (microsoft#24896)
  chore: Expand scope of dependency mismatch generator (microsoft#24880)
  chore: run dependency mismatch generator in release pipeline (microsoft#24881)
  chore: scaffolds react-trigger package (microsoft#24887)
  applying package updates
  chore: a11y docs structure update (microsoft#24871)
  feat: add popupProps to Modal component to allow override internal Popup props (microsoft#24693)
  fix: Set github user in nightly release pipeline (microsoft#24850)
  chore(react-aria): restructure folder organization (microsoft#24884)
  ci(github): fix invalid json string in issues.yml v2 (microsoft#24886)
  Add react-components/unstable to tsconfig aliases (microsoft#24878)
  ...
NotWoods pushed a commit to NotWoods/fluentui that referenced this pull request Nov 18, 2022
Use the openOnHover prop in Menu again. It defaults to isSubmenu but can be overridden.
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.

[Bug]: react-menu no longer opens on hover with openOnHover prop
6 participants