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

Don't treat links to "#" as "/" #285

Merged
merged 2 commits into from
Apr 16, 2013
Merged

Don't treat links to "#" as "/" #285

merged 2 commits into from
Apr 16, 2013

Conversation

DVSoftware
Copy link
Contributor

Breaks many places where "#" is placed as a dummy href and are handled through click events. It can be prevented with stopPropagation() but then it won't propagate to its parents. This fixes my case, but i'm not sure if it's desirable for others.

Breaks many places where "#" is placed as a dummy href and are handled through click events. It can be prevented with stopPropagation() but then it won't propagate to its parents. This fixes my case, but i'm not sure if it's desirable for others.
@justinbmeyer
Copy link
Contributor

You should not have dummy links with #, javascript:// is better but still wrong.

Prevent changing the hash with preventDefault()

On Feb 17, 2013, at 12:55 PM, DVSoftware notifications@github.com wrote:

Breaks many places where "#" is placed as a dummy href and are handled through click events. It can be prevented with stopPropagation() but then it won't propagate to its parents. This fixes my case, but i'm not sure if it's desirable for others.

You can merge this Pull Request by running

git pull https://github.com/DVSoftware/canjs patch-1
Or view, comment on, or merge it at:

#285

Commit Summary

Don't treat links to "#" as "/"
File Changes

M route/pushstate/pushstate.js (18)
Patch Links:

https://github.com/bitovi/canjs/pull/285.patch
https://github.com/bitovi/canjs/pull/285.diff

@DVSoftware
Copy link
Contributor Author

But it still catches them, unless i call stopPropagation(), which in my case breaks catching the outerclicks (i listen to clicks on body, to close modal windows etc), as it doesn't propagate further. I could change to "javascript://" but i kinda still think it's wrong to treat "#" as "/" route (at least with pushstate).

@justinbmeyer
Copy link
Contributor

What do you mean catches them? The route will change unless preventDefault is called, not stopPropagation.

It's not wrong to respond to changing the hash to #. If the hash changes, can.route should respond to it, whatever it is. Otherwise back button and bookmarking won't work as expected.

On Feb 17, 2013, at 1:18 PM, DVSoftware notifications@github.com wrote:

But it still catches them, unless i call stopPropagation(), which in my case breaks catching the outerclicks (i listen to clicks on body, to close modal windows etc), as it doesn't propagate further. I could change to "javascript://" but i kinda still think it's wrong to treat "#" as "/" route (at least with pushstate).


Reply to this email directly or view it on GitHub.

@DVSoftware
Copy link
Contributor Author

Well, the thing is, it does change even if preventDefault is called. At
least it's the case with pushstate, i'm not sure about hashchange as i
don't use it.
18.02.2013. 00.26, "Justin Meyer" notifications@github.com је написао/ла:

What do you mean catches them? The route will change unless preventDefault
is called, not stopPropagation.

It's not wrong to respond to changing the hash to #. If the hash changes,
can.route should respond to it, whatever it is. Otherwise back button and
bookmarking won't work as expected.

On Feb 17, 2013, at 1:18 PM, DVSoftware notifications@github.com wrote:

But it still catches them, unless i call stopPropagation(), which in my
case breaks catching the outerclicks (i listen to clicks on body, to close
modal windows etc), as it doesn't propagate further. I could change to
"javascript://" but i kinda still think it's wrong to treat "#" as "/"
route (at least with pushstate).


Reply to this email directly or view it on GitHub.


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

@DVSoftware
Copy link
Contributor Author

This should do it, instead of checking for '#', i now changed it to check if default is prevented.

@justinbmeyer
Copy link
Contributor

THis looks good.

@DVSoftware
Copy link
Contributor Author

Travis build seems to be stalled for some reason, going to the link says it's actually passed...

daffl added a commit that referenced this pull request Apr 16, 2013
Don't treat links to "#" as "/"
@daffl daffl merged commit 33cbe12 into canjs:master Apr 16, 2013
@DVSoftware DVSoftware deleted the patch-1 branch May 27, 2014 13:23
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.

None yet

3 participants