-
-
Notifications
You must be signed in to change notification settings - Fork 32.1k
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] Isolate more integration tests #17490
Conversation
|
||
expect(getAllByRole('menuitem')[0]).to.have.property('tabIndex', 0); | ||
expect(getAllByRole('menuitem')[1]).to.have.property('tabIndex', -1); | ||
expect(getAllByRole('menuitem')[2]).to.have.property('tabIndex', -1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have some "managed-focus" matcher in mind but want to collect more use cases before abstracting it away.
No bundle size changes comparing e48aeba...16cfeaa |
This comment has been minimized.
This comment has been minimized.
import Menu from '@material-ui/core/Menu'; | ||
import MenuItem from '@material-ui/core/MenuItem'; | ||
|
||
function NestedMenu(props) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Candidate for improvement. It includes no nested menu as I understand them. There are two sibling menus which is not nesting IMO. Also nitpicking on the fact that the "second-menu" is the first in the tree 😄
We probably want to replace this with a test for a cascading menu when we work on #11723
Cherry-picking from #17394 before I'm confident in fixing #16644
This removes commit counting. This is a concern for benchmarks. I have some ideas how we can integrate this smoothly into the test suite but this requires additional work (mostly isolation and if it's possible then only for testing-library tests)