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

Modal: add restoreFocus option #145

Merged
merged 1 commit into from
Mar 7, 2017
Merged

Modal: add restoreFocus option #145

merged 1 commit into from
Mar 7, 2017

Conversation

erykpiast
Copy link
Contributor

@erykpiast erykpiast commented Feb 15, 2017

Allow to specify if last focus should be restored after modal is hidden.

I suppose it may be possible and appreciated to write test for it, like those ones https://github.com/react-bootstrap/react-overlays/blob/master/test/ModalSpec.js#L312, but before I spend a lot of time on it, I'd like to be sure you're OK with this change.

The thing is, it's not always desired to call focus method on thing that was activeElement at the moment modal was shown. I can imagine it's valid in most of use cases, however in some it's not and... I did things I'm really, really ashamed of to workaround this. Please, let my conscience to be cleared.

Allow to specify if last focus should be restored after modal is hidden
@jquense
Copy link
Member

jquense commented Feb 15, 2017

LGTM

@taion
Copy link
Member

taion commented Feb 15, 2017

Should we check for something like autoFocus && restoreFocus? Seems like it'd be odd to restore focus if autoFocus is unset. Alternatively, do we even need restoreFocus as separate from autoFocus?

@erykpiast
Copy link
Contributor Author

So far It was working independently from each other.

@taion
Copy link
Member

taion commented Feb 15, 2017

Should we restore the focus if autoFocus is unset?

@erykpiast
Copy link
Contributor Author

So far you was doing it :P I really think those are separate features. The thing is, that one is optional and another is not. As simple as that. You'll never cover all the use cases of different people, not in such library as Bootstrap. Moreover, changing default behavior may be treated as backward incompatible API change I suppose, so I'm not sure if it's worth it right now. Probably is worth to create GH issue and discuss the problem there.

@taion
Copy link
Member

taion commented Feb 16, 2017

@jquense What do you think? Should we restore focus by default if the user only disables autoFocus?

@taion
Copy link
Member

taion commented Mar 7, 2017

@jquense ping

@jquense
Copy link
Member

jquense commented Mar 7, 2017

I feel fine with them all being seperate toggles, they are really last ditch escape hatches anyway, so it seems ok to mix and match as you please. Most shouldn't turn any of them off

@taion taion merged commit 9d12911 into react-bootstrap:master Mar 7, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants