Enable use of middle click to close tabs #2190
Conversation
by adding a onMouseDown directive to both tab file icon and tab name and checking the resulting React.MouseEvent for a middle click
by middle clicking and let the tab itself handle the logic for it by checking React.MouseEvent.button value
Please put |
keforbes
changed the title
|
Feel free to edit the title and remove |
|
Thank you for the PR, @texhnolyze ! The functionality change looks great to me 👍 Would it be possible to add a test case to exercise this behavior? I'm wondering if we could use the simulate method to exercise the normal click and non-middle button click, and check if the correct EDIT: Sorry I should've read the PR description, sounds like you were already thinking about this 😄 Awesome!
As long as the middle-click behaves the same as clicking the 'x' in this case, that should be fine for this change IMO. We could track changes to improving the |
|
@keforbes thanks, will do in the future |
and write own @types module declaration so that it can be used in testing nstead of using a mock. The mock does not suffice as the way the actual module previously had to be imported was "import * as classNames" where classNames was the actual function. It is not possible to build a module in typescript/es6 imports, which will directly return a function in the same way the dependency did in commonjs module syntax. Instead when defining a function to be returned as default export it is returned as an Object like this "{ default: [Function] }", which is correctly resolved when importing with "import classNames from 'classnames'". This only previously worked in production as webpacks ts-loader, handles this issue, whereas when testing the sources are only compiled with tsc. There is an update to the classnames dependency on the current master, but there hasn't been a release since 2006. So options were to setup webpack for tests as well or add updated classnames dependency which sets a "default" value on its commonjs exports. Links for reference: JedWatson/classnames#152 JedWatson/classnames#106 DefinitelyTyped/DefinitelyTyped#25206 microsoft/TypeScript#2719
Codecov Report
@@ Coverage Diff @@
## master #2190 +/- ##
==========================================
- Coverage 37.14% 37.13% -0.01%
==========================================
Files 296 296
Lines 12137 12138 +1
Branches 1597 1599 +2
==========================================
Hits 4508 4508
- Misses 7378 7379 +1
Partials 251 251
Continue to review full report at Codecov.
|
| clickedTab | ||
| .find(".corner") | ||
| .first() | ||
| .simulate("mouseDown", { button: 1 }) |
There was a problem hiding this comment.
Wow, thanks for figuring out how to exercise this @texhnolyze 💯
|
@texhnolyze - sorry for the late response - thanks for expanding the test coverage! I'm glad you found a way to mock the I'm ready to bring this in when you are - let me know if it's good to go. Thanks again! |
|
@bryphe yeah took my some trial and error, but this is finished from my side for this pull request, if you already had a look at the changes you probably saw what I did with the This works for now, but I would suggest maybe dropping the dependency in favor of an own implementation, because as it currently stands it does not seem like there will be a new release soon. The implementation of this as a project component should be failrly trivial. What do you think? Also now that I know a bit more about the test setup and jest and so on, I'll do a pull request, when I find the time, to add a bit more test coverage and to get to know the code base better. |
texhnolyze
changed the title
Thanks for calling this out!
Yes, it seems reasonable to me for now, but as you mentioned, I think long-term it makes sense to move away from this. With the direction the codebase is moving, I think, instead of using css classes directly, we could refactor those classes to use
Awesome! That would be great 💯 Really appreciate the help! |
|
Merged - thank you for the contribution, @texhnolyze ! 💯 |
Enables closing oni tabs with middle click, much like e.g. firefox/chrome.
Corresponds to #2069.
Do not merge yet:
As mentioned in the issue there is still some clarification needed
concerning the ability to close tabs, when only a single one is opened and how to go
about testing clicks/middle clicks of tabs to ensure they are correctly selected and closed.