-
Notifications
You must be signed in to change notification settings - Fork 23
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
Properly open selected folder on middle click #612
base: main
Are you sure you want to change the base?
Conversation
426bdc9
to
1c6783f
Compare
Noticed I missed ctrl + click, will amend |
1c6783f
to
350654e
Compare
There is indeed no way to monitor the browser for multiple tabs in CDP. We also don't do this anywhere as we don't really want people to have a lot of parallel cockpit sessions in multiple tabs - we never test this, and at some point the communication overhead becomes quite severe. I triggered the tests, I'll leave the functional review to @jelly . Thank you! |
if (newTab) { | ||
open(`#/?path=${newPath}`); |
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.
These 2 added lines are not executed by any test.
@@ -194,7 +198,7 @@ export const FilesCardBody = ({ | |||
} | |||
|
|||
if (ev.detail > 1) { | |||
onDoubleClickNavigate(file); | |||
onClickNavigate(file); |
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 added line is not executed by any test.
} | ||
}; | ||
|
||
const handleAuxClick = (ev: MouseEvent) => { |
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 added line is not executed by any test.
if (ev.button === 1) { | ||
ev.preventDefault(); | ||
const name = getFilenameForEvent(ev); | ||
const file = sortedFiles?.find(file => file.name === name); |
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.
These 4 added lines are not executed by any test.
if (file) { | ||
setSelected([file]); | ||
onClickNavigate(file, true); |
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.
These 3 added lines are not executed by any test.
Thanks Martin, yea, I guess a middle ground is merging the middle click, which works as expected, I didn't really put much thought into CTRL + CLICK, as it currently fights with multiple selection, just wanted to showcase that. |
@jelly can I get a review? :), do not mind the ctrl click, I think we need to discuss it further, but if you are happy I can remove it and we could partially work only the middle click |
Now I have no clue how to test this, is there an easy way to figure out if the browser opened a new tab?
Fixes #545