-
Notifications
You must be signed in to change notification settings - Fork 7.6k
Added support for closing files on middle click #3901
Conversation
// Update the listItem's apperance | ||
_updateFileStatusIcon($newItem, isOpenAndDirty(file), false); | ||
_updateListItemSelection($newItem, curDoc); | ||
|
||
$newItem.mousedown(function (e) { | ||
_reorderListItem(e, $(this)); | ||
if (e.which === 2) { | ||
_handleMiddleMouseClick(e); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This does work, but I think this should be done in mouse up events rather than right here. Try with tabs in the browsers and other editors. All of them work on mouse ups.
@RaymondLim fixes pushed. |
@@ -370,6 +380,13 @@ define(function (require, exports, module) { | |||
_reorderListItem(e, $(this)); | |||
e.preventDefault(); | |||
}); | |||
|
|||
$newItem.on("click", function (e) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: can you change it to .click()
so that it is consistent in coding style with existing code? We have .hover
and .mousedown
.
@RaymondLim change pushed. |
@@ -370,6 +380,13 @@ define(function (require, exports, module) { | |||
_reorderListItem(e, $(this)); | |||
e.preventDefault(); | |||
}); | |||
|
|||
$newItem.click(function (e) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a lot easier to implement by just changing line 233 to if (!fromClose || event.which !== 2)
since mousedown calls _reorderListItem
, and inside _reorderListItem
if the user isn't using the left button, it calls drop
, so if in drop
we check if the user pressed the middle button and close the file, it works.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@TomMalbran
Thanks for your suggestion of an alternative solution which has the least changes to get this functionality. But I think @WebsiteDeveloper current solution is cleaner, and easier to understand and easier to locate the implementation due to the function name and the event being used. Your solution may not be easy for someone to locate the code since it is harder to imagine that a middle button click is implemented inside drop() function which is called from _reorderListItem
. And also if someone is going to restrict the call of drop() to only left mouse button clicks in the future (since drag-n-drop is done with left mouse button), then this functionality will be broken.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@RaymondLim No problem. I thought it would make sense to have all the close implementations together, since closing from the "x" button is already implemented inside the drag and drop implementation to be able to drag from the close button if desired.
Thanks @WebsiteDeveloper ! Merging. |
Added support for closing files on middle click
Implemented as requested in #3889