-
Notifications
You must be signed in to change notification settings - Fork 37
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! |
src/files-card-body.tsx
Outdated
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.
src/files-card-body.tsx
Outdated
@@ -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.
src/files-card-body.tsx
Outdated
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.
src/files-card-body.tsx
Outdated
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 |
Thanks! However, this doesn't work as expected:
In other words, I would expect it to open in the background like other tabs and be within Cockpit. |
Not able to find anything that would work everywhere, if anything chrome still seems to be the issue: https://stackoverflow.com/a/76529021 whether that is intended or not, I am not sure. |
Err... uh... what? That code looks ridiculous. Creating an anchor element from a button? 😕 Why not just use an actual anchor element within the DOM and use that, like intended?
Control click gets harder — normally I'd suggest that work too, but file managers generally have control-click for multi-select. Basically you'd ususally do something like this with HTML + JavaScript like this simple example: <div id="fileList">
<a href="Documents/" class="folder">Documents</a>
<a href="Projects/" class="folder">Projects</a>
<a href="document1.txt" class="file">document1.txt</a>
<a href="image.jpg" class="file">image.jpg</a>
<a href="archive.zip" class="file">archive.zip</a>
</div> document.getElementById('fileList').addEventListener('click', event => {
// On folders, let a middle click go through and be handled natively
if (event.target.matches('.folder') && event.button === 4) return;
// Don't follow the link and instead do other things below
event.preventDefault();
// Do stuff to files and folders
if (event.target.matches('.file, .folder')) {
console.log('Clicked:', event.target.href);
}
}); (It basically is using event delegation on the parent of the folders, then checks if it's a folder and if middle click was active, then returns if so, otherwise it prevents default actions and does stuff to files and folders.) I haven't tested the JS, but it probably works. If I type something wrong, pretend it is pseudocode. 😉 (Hopefully something like the above can be adapted.) |
Also middle click on my end seems to not open in foreground, I am using chrome / windows, even with my changes (I am ignoring ctrl click).
Also make sense, will give a try |
Ah, I'm using Firefox on Linux. I tried Chromium and see the tabs open in the background as expected. Weird that this is different! I just tried GNOME Web (WebKit, like Safari) and middle clicking does absolutely nothing at all. No tabs at all, nor navigation. |
350654e
to
45f8068
Compare
Hi @garrett, can you take another look? I did test on both chrome and firefox and respects the background opening just fine, but of course in list view, it only triggers from the |
friendly ping @garrett |
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.
Looks good. I have a simple question about the JS; replied inline.
45f8068
to
4d76710
Compare
@garrett one last check? |
4d76710
to
a7b0961
Compare
Updated to match path changes from #678 |
It looks fine to me, but we had a philosophical question about enabling this, even at this level, and some high priority things came up meanwhile. So... I'm not sure at the moment. |
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