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.

Sign up for GitHub

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

Jump to bottom

MenuList integration tests have dependencies on instance methods #14756

Closed
2 tasks done
ryancogswell opened this issue Mar 6, 2019 · 0 comments
Closed
2 tasks done

MenuList integration tests have dependencies on instance methods #14756

ryancogswell opened this issue Mar 6, 2019 · 0 comments
Labels
component: list This is the name of the generic UI component, not the React module! component: menu This is the name of the generic UI component, not the React module!

Comments

@ryancogswell
Copy link
Contributor

The MenuList integration tests currently have dependencies on three different instance methods: focus, setTabIndex, and resetTabIndex. The focus method appears to have been added to MenuList purely for testing reasons and can be fully removed once the test is no longer dependent on it. The other two instance methods were being used to force a condition to get to 100% test coverage, but it is also possible to hit that condition by putting focus to a MenuItem before the MenuList componentDidMount executes.

  • This is not a v0.x issue.
  • I have searched the issues of this repository and believe that this is not a duplicate.

Context 🔦

As part of the work for #8191 I'm going to be converting MenuList to a function component instead of a class. Instance methods will no longer be accessible at that point. This is one nice benefit of converting to function components -- it reduces the surface area of internal details that are possible for developers to abuse. Even though none of the instance methods are documented, with enough developers using Material-UI someone will use some of those instance methods for some odd reason just because they exist.

I will be submitting a pull request within the next day that addresses this.

Your Environment 🌎

Tech Version
Material-UI next
ryancogswell added a commit to ryancogswell/material-ui that referenced this issue Mar 6, 2019
@ryancogswell
[MenuList] Remove focus method and test dependencies on instance meth…
f4137ce 
…ods (mui#14756)

* Remove test dependencies on focus, setTabIndex, and resetTabIndex

* Remove focus method from MenuList

* Increase MenuList branch coverage from 94.64% to 100%
@ryancogswell ryancogswell mentioned this issue Mar 6, 2019
Merged
1 task
oliviertassinari pushed a commit that referenced this issue Mar 7, 2019
@ryancogswell @oliviertassinari
[MenuList] Remove focus method and test dependencies on instance meth…
0c5342e 
…ods (#14757)

* [MenuList] Remove focus method and test dependencies on instance methods (#14756)

* Remove test dependencies on focus, setTabIndex, and resetTabIndex

* Remove focus method from MenuList

* Increase MenuList branch coverage from 94.64% to 100%

* [test] Formatting fixes (yarn prettier)

* [test] Code consistency fixes
@oliviertassinari oliviertassinari added the status: waiting for maintainer These issues haven't been looked at yet by a maintainer label Dec 20, 2022
@zannager zannager added component: list This is the name of the generic UI component, not the React module! component: menu This is the name of the generic UI component, not the React module! and removed status: waiting for maintainer These issues haven't been looked at yet by a maintainer labels Jan 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
component: list This is the name of the generic UI component, not the React module! component: menu This is the name of the generic UI component, not the React module!
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@ryancogswell @oliviertassinari @zannager