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

[Menu] Convert to function component #15068

Merged
merged 4 commits into from
Mar 28, 2019

Conversation

joshwooding
Copy link
Member

@joshwooding joshwooding added new feature New feature or request component: menu This is the name of the generic UI component, not the React module! labels Mar 26, 2019
@joshwooding
Copy link
Member Author

joshwooding commented Mar 26, 2019

I'm expecting the last three tests to fail.

  • should call props.onEntering with element if exists
    • I already check something is passed to onEntering in the handler tests
  • should call props.onEntering, disableAutoFocusItem
    • I can check something is passed to onEntering but I'm unsure why disabledAutoFocusItem would affect onEntering
  • call handleListKeyDown without onClose prop
    • No idea how to test this one, do we need to?

@oliviertassinari ?

packages/material-ui/src/Menu/Menu.js Outdated Show resolved Hide resolved
packages/material-ui/src/Menu/Menu.js Outdated Show resolved Hide resolved
@mui-pr-bot
Copy link

mui-pr-bot commented Mar 26, 2019

@material-ui/core: parsed: -0.24% 😍, gzip: -0.04% 😍

Details of bundle changes.

Comparing: b3e4c49...be54b64

bundle parsed diff gzip diff prev parsed current parsed prev gzip current gzip
@material-ui/core -0.24% -0.04% 349,795 348,943 90,100 90,065
@material-ui/core/Paper -0.00% -0.05% 67,869 67,866 19,832 19,822
@material-ui/core/Paper.esm -0.00% -0.02% 60,201 60,198 18,572 18,568
@material-ui/core/Popper +0.01% 🔺 -0.02% 30,460 30,463 10,528 10,526
@material-ui/core/styles/createMuiTheme 0.00% 0.00% 17,384 17,384 5,726 5,726
@material-ui/core/useMediaQuery 0.00% 0.00% 2,469 2,469 1,043 1,043
@material-ui/lab +0.01% 🔺 -0.02% 148,712 148,720 43,698 43,691
@material-ui/styles -0.01% -0.06% 53,102 53,099 15,444 15,435
@material-ui/system 0.00% 0.00% 17,136 17,136 4,527 4,527
Button -0.00% -0.01% 88,356 88,355 26,177 26,175
Modal 0.00% -0.00% 82,460 82,461 24,670 24,669
colorManipulator 0.00% 0.00% 3,232 3,232 1,299 1,299
docs.landing 0.00% 0.00% 49,958 49,958 10,837 10,837
docs.main -0.14% -0.01% 646,389 645,467 200,783 200,756
packages/material-ui/build/umd/material-ui.production.min.js -0.26% -0.09% 298,354 297,587 82,635 82,559

Generated by 🚫 dangerJS against be54b64

@oliviertassinari
Copy link
Member

@ryancogswell I hope this change doesn't conflict with your effort. Let us know.

@oliviertassinari
Copy link
Member

@joshwooding I have given a shot to the problem. I have removed the did mount logic. It works great without. I have removed the .instance() usage in the tests.

@ryancogswell
Copy link
Contributor

I hope this change doesn't conflict with your effort. Let us know.

@oliviertassinari So far, my work has only been in MenuList and its tests, but I will be working in Menu and its tests soon. As long as this is merged soon it won’t cause me any issues.

@oliviertassinari oliviertassinari marked this pull request as ready for review March 27, 2019 17:46
@oliviertassinari oliviertassinari merged commit 12fa8c0 into mui:next Mar 28, 2019
@joshwooding joshwooding deleted the menu-function-component branch March 31, 2019 20:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: menu This is the name of the generic UI component, not the React module! new feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants