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] Add prop to disable auto focus #11984

Merged
merged 3 commits into from
Jun 26, 2018
Merged

[Menu] Add prop to disable auto focus #11984

merged 3 commits into from
Jun 26, 2018

Conversation

th317erd
Copy link
Contributor

I keep a fork of material ui simply and only because material ui Menu forcefully updates the element focus. I would greatly appreciate the merging of this PR to allow disabling this "feature". Thank you!

@oliviertassinari oliviertassinari added the PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI label Jun 26, 2018
@oliviertassinari
Copy link
Member

@th317erd The concept sounds good to me. However, we need to find a different wording, it's conflicting with:
https://github.com/mui-org/material-ui/blob/91ecbbd79dca4a2432157e79e6f4f5e2c9f8dfc6/packages/material-ui/src/Modal/Modal.js#L336

@th317erd
Copy link
Contributor Author

th317erd commented Jun 26, 2018

@oliviertassinari That is exactly why I chose that prop name. I originally had it as autoFocus: false, but I needed the same thing to happen in Popover, and naming it this way allows it to be passed the as the same name via other props. I believe we WANT to have this prop pass though, because the intent is clear for the Menu, Popover, and Modal: don't steal my focus. Thoughts?

@oliviertassinari oliviertassinari changed the title Added Menu prop to disable auto focus [Menu] Add prop to disable auto focus Jun 26, 2018
@oliviertassinari oliviertassinari added new feature New feature or request component: menu This is the name of the generic UI component, not the React module! labels Jun 26, 2018
@oliviertassinari
Copy link
Member

oliviertassinari commented Jun 26, 2018

@th317erd Let's say we can this pull request property disableAutoFocusSelectedItem. Then I see a use case for disableAutoFocusSelectedItem={true} and disableAutoFocus={false}. A developer might want to move the focus to the menu without focusing on the first item, this way, a user can use the keyword to focus an item without having an item focused by default.

@th317erd
Copy link
Contributor Author

@oliviertassinari Good thought! You are correct. I just pushed a new commit that renames this property. Thank you for your thoughts on this. :)

@oliviertassinari oliviertassinari removed the PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI label Jun 26, 2018
@oliviertassinari oliviertassinari merged commit 09896d4 into mui:master Jun 26, 2018
@oliviertassinari
Copy link
Member

@th317erd Thank you :)

acroyear pushed a commit to acroyear/material-ui that referenced this pull request Jul 14, 2018
* Added Menu prop to disable auto focus

* Update prop name per request of oliviertassinari

* ready to be merged
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.

2 participants