Skip to content
This repository has been archived by the owner on Nov 3, 2021. It is now read-only.

Proposed fix for bug 921317 (using preventDefault) #14588

Closed
wants to merge 0 commits into from
Closed

Proposed fix for bug 921317 (using preventDefault) #14588

wants to merge 0 commits into from

Conversation

dholbert
Copy link
Contributor

@gnarf
Copy link
Contributor

gnarf commented Dec 11, 2013

Glad you've figured out the actual root cause of the problem, however, I noticed that the #alarm-edit-panel links – which aren't in the "tabs", and therefore don't go through this preventDefault – also don't cause the problem.

It seems if we start the transition/animation inside the hashchange event the problem also disappears.

If we remove bind to this.tabs.on('selected',...) from the app.js init method, the hashchange method still kicks off the navigate and everything seems clean.

I'm torn on which is the better solution. I think I prefer having the location.hash actually change, and rely on the hashchange instead of the selected event on tabs. cc @jugglinmike for a second opinion

@dholbert
Copy link
Contributor Author

It seems if we start the transition/animation inside the hashchange event the problem also disappears.

Cool -- that makes sense, and that seems like a cleaner solution.

(The problem only arises when the animation starts before we navigate. So if we explicitly wait to kick off the animation until the hashchange has occurred, it makes sense that we'd avoid the problem.)

@jugglinmike
Copy link
Contributor

@gnarf I agree--using the hashchange event seems to jive better semantically.

@dholbert
Copy link
Contributor Author

Cool. @gnarf, feel free to take this if you already have a patch along those lines then.

@gnarf
Copy link
Contributor

gnarf commented Dec 11, 2013

Will do. Thanks again for the most excellent find of the root cause here!
On Dec 11, 2013 4:50 PM, "Daniel Holbert" notifications@github.com wrote:

Cool. @gnarf https://github.com/gnarf, feel free to take this if you
already have a patch along those lines then.


Reply to this email directly or view it on GitHubhttps://github.com//pull/14588#issuecomment-30372749
.

@jugglinmike
Copy link
Contributor

Agree with @gnarf again: great work @dholbert -- really interesting stuff :)

@dholbert
Copy link
Contributor Author

Thanks! :)

@try-server-hook
Copy link

dholbert Daniel Holbert (dholbert) started tests. Results

@dholbert dholbert closed this Jul 24, 2014
@dholbert
Copy link
Contributor Author

(Sorry for the spam -- updated my clone without closing this pull request, which apparently triggered tests. My git/github-fu is poor but improving.)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
4 participants