nsqadmin: allow ctrl/meta+click to open new tab #841
Conversation
|
is it any good? |
|
I only play a javascript developer on TV, but it seems to work for me. |
| if (e.ctrlKey || e.metaKey) { | ||
| // allow ctrl+click to open in a new tab | ||
| return true | ||
| } | ||
| e.preventDefault(); | ||
| e.stopPropagation(); |
There was a problem hiding this comment.
If you were stopping the event propagation so that the click isn't registered on another element, I'm guessing you'll still want to do that even when ctrl+clicking. Maybe move the stopPropagation above the if clause?
| @@ -136,6 +136,10 @@ var AppView = BaseView.extend({ | |||
| }, | |||
|
|
|||
| onLinkClick: function(e) { | |||
| if (e.ctrlKey || e.metaKey) { | |||
| // allow ctrl+click to open in a new tab | |||
| return true | |||
There was a problem hiding this comment.
I don't know if you actually need to return true here.
Also, semicolon usage and indentation isn't consistent with rest of file.
There was a problem hiding this comment.
I'm not sure i follow. Are you implying an empty return?
return false should stop the browser navigation to the href. That would mean clicking does nothing. return true (from experimentation) allows the browser to navigate and do a full page load on the href target (though because ctrl or meta, i expect the browser to be doing that in a new tab)
Why would i want to stopPropagation?
There was a problem hiding this comment.
(also good call on missing semicolon)
There was a problem hiding this comment.
I'm not sure i follow. Are you implying an empty
return?
Yeah, I think it can just be an empty return. I believe jQuery only cares if you return false - in which case it's the same as calling stopPropagation and preventDefault. This might be helpful.
Why would i want to
stopPropagation?
I'm not sure why it's being called right now. I'm just saying if there's a good reason to call it, it probably should be called even when ctrl-clicking. I'm assuming it's there for some reason, but I may be wrong. I'm not too familiar with this codebase.
There was a problem hiding this comment.
fixed to return; I don't think we actually need stopPropagation so going to choose to omit that.
jehiah
force-pushed
the
nsqadmin_click_841
branch
from
6abf7e3 to
becb682
Compare
|
RFM @mreiferson |
Often when browsing nsqadmin i want to ctrl+click something to compare two topics or channels in different tabs, but the current behaviour disables that capability.
This causes clicks to be handled normally by the browser when ctrl/meta is pressed instead of doing any fancy SPA navigation.
RFR @rolyatmax @mreiferson