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

browser history #19

Closed
wants to merge 0 commits into from
Closed

browser history #19

wants to merge 0 commits into from

Conversation

lauxley
Copy link
Contributor

@lauxley lauxley commented Jan 31, 2013

Note that there is a bug in DOMWindow which make this fail when one close the window with the close button (not with escape or with a click on the overlay)

@souen
Copy link
Contributor

souen commented Jan 31, 2013

👍 Souen Boniface likes this !

function bindHistory() {
jQuery(window).bind('popstate', function(e) {
// close the window in case we hit the back button
$.fn.closeDOMWindow();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not passing settings to the closeDOMWindow() call will be a problem: Not only will that break some custom usage of DOMWindow (since the call will use an empty object for settings) but it means our own functionCallOnClose won't be called. At the very leasts this means the CSS overflow property won't properly be reset on the body.

I'm not sure we can easily solve this without modifying/removing DOMWindow :(

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is DOMWindow supposed to work with more than one window ? if not we could use a global variable for settings, it still wouldn't be a good design but at least would fix this without too much hassle. I think it would be better than passing the settings every time, which doesn't really make sense anyway ?

@diox diox mentioned this pull request Feb 4, 2013
@lauxley
Copy link
Contributor Author

lauxley commented Mar 13, 2013

Closing this for a more practical and clean pull request see #25

@lauxley lauxley closed this Mar 13, 2013
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