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

Enforce focus for most recent modal only, fixes #4781 (even though it has been closed already) #5181

Closed
wants to merge 2 commits into from

Conversation

everilae
Copy link

In my opinion the focus grabbing browser crash in Firefox is not acceptable, even if showing multiple modals is bad UI behaviour.

@andrewdeandrade
Copy link

Hey @everilae,

Thanks for opening this pull-request! Unfortunately, it looks like it fails to pass the criteria neccessary for submitting to bootstrap. The following things are currently failing:

  • should always be made against -wip branches
  • should always be made from feature branches
  • should always include a unit test if changing js files

For a full list of issue filing guidelines, please refer to the bootstrap issue filing guidelines.

thanks!

@stephankaag
Copy link

Made a new pull request @ #5833

@fat
Copy link
Member

fat commented Dec 20, 2012

don't want to keep track of all the modals in memory – this seems very inefficient, particularly at an instance level

@fat fat closed this Dec 20, 2012
@everilae
Copy link
Author

Though I've given up on this a long ago (implemented the fix locally), doesn't the code as it is now store references in an array? The modals are "in memory" already through jq data binding to elements. No big loss there, if I've understood anything.

If inefficient at any point, then at the inArray perhaps. But the idea wasn't to pop up 100 modals, but allow a few errors to overlap. Even 10 error dialogs overlapping would mean that everything has gone to hell anyway, at least in our case.

Also the store is temporary, since the instance references are spliced away on hide. Did "all modals in memory" perhaps mean that this part was missed?

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants