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

Conversation

Projects
None yet
3 participants
@DVSoftware
Contributor

DVSoftware commented Feb 17, 2013

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.

Don't treat links to "#" as "/"
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

This comment has been minimized.

Show comment
Hide comment
@justinbmeyer

justinbmeyer Feb 17, 2013

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

Contributor

justinbmeyer commented Feb 17, 2013

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

This comment has been minimized.

Show comment
Hide comment
@DVSoftware

DVSoftware Feb 17, 2013

Contributor

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).

Contributor

DVSoftware commented Feb 17, 2013

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

This comment has been minimized.

Show comment
Hide comment
@justinbmeyer

justinbmeyer Feb 17, 2013

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.

Contributor

justinbmeyer commented Feb 17, 2013

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

This comment has been minimized.

Show comment
Hide comment
@DVSoftware

DVSoftware Feb 17, 2013

Contributor

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/bitovi/canjs/pull/285#issuecomment-13700179.

Contributor

DVSoftware commented Feb 17, 2013

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/bitovi/canjs/pull/285#issuecomment-13700179.

@DVSoftware

This comment has been minimized.

Show comment
Hide comment
@DVSoftware

DVSoftware Feb 18, 2013

Contributor

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

Contributor

DVSoftware commented Feb 18, 2013

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

@justinbmeyer

This comment has been minimized.

Show comment
Hide comment
@justinbmeyer

justinbmeyer Mar 15, 2013

Contributor

THis looks good.

Contributor

justinbmeyer commented Mar 15, 2013

THis looks good.

@DVSoftware

This comment has been minimized.

Show comment
Hide comment
@DVSoftware

DVSoftware Mar 15, 2013

Contributor

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

Contributor

DVSoftware commented Mar 15, 2013

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

Merge pull request #285 from DVSoftware/patch-1
Don't treat links to "#" as "/"

@daffl daffl merged commit 33cbe12 into canjs:master Apr 16, 2013

1 check was pending

default The Travis build is in progress
Details

@DVSoftware DVSoftware deleted the DVSoftware:patch-1 branch May 27, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment