-
Notifications
You must be signed in to change notification settings - Fork 12
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
FileBrowserView adjustments #1666
Conversation
if (asset) { return; } | ||
location.value = path; | ||
function itemIsViewable(item: AssetPath) { | ||
const nonViewableExtensions = ['nwb', 'zip', 'gz', 'nii', 'tif', 'tiff', 'avi']; |
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.
I was surprised to discover that .tiff
is not supported by most of the browsers. It is supported by Safari though. But I also wonder if/how it would interact with some browser extensions which might be there to support viewing those types like https://www.blackice.com/broadcast/10132020TIFFV/index.html ?
@yarikoptic I didn't know tif was supported in Safari. I have removed .tif and .tiff from the non-viewable in my PR. |
ping @waxlamp |
Thank you for the pull request, @magland! I reviewed this with @AlmightyYakob and our collective feedback is below.
Great!
Two thoughts:
Essentially, my recommendation is to remove this portion of the PR in favor of doing nothing and letting the browser handle it (my preference) or examining this more organically and doing the right thing with mimetypes etc. Thoughts?
Some history: at some point, I asked our frontend devs to remove the material design "floop" that occurs on click precisely for the reason you stated--the violation of expectation of something happening was bad UX that I thought we could fix easily. It turns out that the way the logic is structured, this wasn't so easy so we left it as is. We actually had plans to do something like open an info panel containing rendered metadata or something like that, but it was never in demand enough to actually do it. Furthermore, I tend to agree with you that clicking on the item should initiate "opening" it, whatever that means. So, our recommendation is to use a click on the item itself as the "view" action, and also to get rid of the eyeball icon altogether. The eyeball icon has always struck me as kind of gross (even though I'm the one who introduced it) and I think rather than always displaying it, or conditionally displaying it (as in your example) the best thing is to get rid of it altogether and let the natural UX of clicking directly on the item to be the path to "opening" it for view. This also leaves open the opportunity to still do a nicely rendered metadata display for the "info" icon (in place of opening a separate window showing the API docs page for the metadata, but that's really a separate issue). |
Thanks for the review! @waxlamp I understand your points, and how you don't want to maintain lists of file extensions with different behaviors. However, DANDI is built around certain types if I am understanding the project, including NWB. We know that browsers will never natively support this format. If a user clicks on the view button of a NWB file (or clicks the file in the case where you remove the eyeball), I don't think they would expect it to start downloading - especially since there is a separate download button. And I don't think that's a recommended action anyway, since the NWB files can be tens or even hundreds of GB in size. Having to cancel a download can be annoying when all the user wanted to do was get a preview of the file. As an aside, my preference would be to have the view button (or the file click) open in Neurosift in a separate tab. I have met several people who didn't know that Neurosift was an option because it's hidden in the menu dropdown, but are now grateful to be able to preview the contents of NWB files. One person in particular told me he had been downloading terrabytes of data just so he could inspect the metadata of those files. |
In "principle" I am with @waxlamp on this -- it is nice to give a client (browser) a chance to handle correctly some fancy file type via e.g. some extension to support that file type and show in the browser. And I hate when that is not done e.g. for .pdf or .json files which could be handled either by Acroread of even natively by the browser (for .json), and it is not infeasible to see some But "pragmatically", given current state (AFAIK - no such browser built-in capabilities, or extensions in popular use), I would prefer cleaner interface/actions and thus a combination of
IMHO that would provide the best UX to be able to view whenever it is possible. |
@magland thank you again for preparing this PR, and sorry for the long delay in processing it (working on this!). I'm closing this PR in favor of these PRs:
which cover most of the uncontroversial ideas in this PR. I've also opened #1848 to track a good but slightly controversial idea that we didn't resolve here. A quick note on process for the future: placing small but technically unrelated ideas into separate PRs (even if the ideas share a theme) will make it much easier to review and accept uncontroversial contributions while leaving space to argue about the controversial ones. |
Thanks @waxlamp that makes sense. Looking forward to seeing the updates! |
Here I propose a few modifications to the FileBrowserView component.
Add some tooltips for the actions
"View asset", "Download asset", "View asset metadata", "Open in external service"
Do not show the view/eye action if the path is a non-viewable type, i.e., the browser doesn't support it
Currently, when the user clicks to view an asset, and the browser does not support it inline (e.g., .nwb), then the behavior is equivalent to the download action (it downloads the file). Therefore I propose to hide the view action button for these cases. Rather than providing a list of supported types, I decided to take a more conservative approach and provide a list of non-viewable types, the most important being '.nwb'.
View the item on click
I think the user would expect some action on clicking the asset name (I personally keep expecting that even though I've used the system for many months). So, in the case where the item is viewable, I propose to initiate the view action on clicking the asset name. In order to make this work properly, I needed to add @click.stop attributes to the button actions so that clicking those does not propagate up to the parent item.
Here's a screenshot showing how some assets are viewable and others are not: