don't mark links visited on right click or on drag #1224
Conversation
|
Thanks for the patch! This is related to #79. |
|
apologies for not searching the issues, I figured since it'd been going on so long it probably wasn't reported! Good find :) |
|
Oh no, not your fault. I just wanted to put a link here so we remember to close it. |
|
Hmm, looks like some of the issues in #78 wouldn't be addressed by this (dragging the link around the page with either the middle or left mouse button is counted as a visit.) Is the TBH I'm surprised that there isn't a standard DOM event for when navigation is caused by interacting with an anchor... |
|
the click event might be more appropriate, yes. I had assumed there was some reason you'd used mousedown - perhaps you'd found that the event was sometimes lost with timing due to the few MS difference. Detecting a click prior to immediate navigation away from the site is a tricky one at times depending on the browser (although maybe with newer browsers that has become less problematic, I'm not sure) Do you know why mousedown was chosen, by chance? certainly some logic could be put in to my patch to handle the drag scenario but that wasn't on my radar at the time I wrote this up. I feel like that's a much tinier percentage use case that might be addressed in a second round, but if you'd like me to take a look at it I can! |
|
I don't know if there was any reason for using |
|
okay, I've changed it to click! |
|
LGTM, but I'll defer to one of the JS folks 🐟 |
|
my above confession still stands, haven't had time to recreate a VM yet, so if someone could give it a sanity test real fast that'd be appreciated. |
honestbleeps
changed the title
|
👓 @ajacksified @madbook @dwick (the frontend crew) Note, a lot of people are on vacation right now so I doubt there'll be much movement until next week. Sorry :( |
| @@ -15,8 +15,8 @@ r.visited = { | |||
| }, | |||
|
|
|||
| onVisit: function(ev) { | |||
| if (ev.type == 'keydown' && ev.which != 13) { | |||
| // only handle enter key presses | |||
| if ((ev.type == 'keydown' && ev.which != 13) || (ev.which === 3)) { | |||
There was a problem hiding this comment.
nit: (ev.which === 3) doesn't require braces
There was a problem hiding this comment.
alright, removed
|
looks good to me 🐟 |
|
anything else needed, @ajacksified @dwick or @madbook ? I know it's tiny, but I'd love to get my first (accepted) reddit PR in :-) |
|
Hey @honestbleeps! Thanks for the contribution, it's been rebased and merged as ad42be6. 🏆 incoming! |
|
:-D thanks! very exciting |
|
Can you email me your reddit username? wick@reddit.com |
|
@dwick It's honestbleeps. :) |
|
Great work @honestbleeps - have a high five on me with @changetip |
|
Hi @honestbleeps, I've delivered a Bitcoin tip worth a high five (24,011 bits/$5.00) from @gorillamania to your ChangeTip wallet. |
a common use case, for example, is right click / open in incognito. it's not good to add links to history when that's the user's intent, so this function shouldn't be run on right clicks.
confession: this is a blind-ish PR I whipped up real fast without testing because my VM is dead. If someone with a running VM could test this, I'd appreciate it. Otherwise, I'll try to get it running again soon.
I do think it should work though. Pretty simple change.