dereklu888 commented 3 years ago

Currently, only the basic context menu is implemented, I'll be adding rendering functionality soon.

dereklu888 commented 3 years ago

Some issues I'll still need to clear up (not an exhaustive list, just some things I thought I'd note down):

how to resolve if there are 2 links in the same row, one that goes to an internal page and another that goes to an external website (or just use the default browser context menu)
downloading the libraries used and hosting it locally rather than pulling it from the internet
whether there should be more options available in the context menu, like opening in a new window or others.

yarikoptic commented 3 years ago

Thanks for posting preview to http://datasets-dev.datalad.org/ so I had chance to try it out. Here is what I think we should do:

Let's not overload right-click (so users can still resort to what browser provides them with) but prepend each path with ⋮ (in grey, may be there is a better symbol or icon to make it more obvious..) which would bring up the contextual menu
- I think it is ok to keep "Copy Link" but I think "Open Link" isn't worth keeping there since that is the regular action happening on clicking the entry
Remove "open in new tab" -- with above people could resort to the browser configuration/shortcuts
For folders/subdatasets
- there should be no "View in renderer" (for now at least)
- if that folder is present locally (we have information since we do cross-out those which aren't), add "Open folder" and harmonize "Visit folder" to be also "Open folder". If it could be made to just be an <a> within the menu - then they could again resort to browser features to open in new tab or not. If it can't be <a> -- then ok, to not bother with "in new tab" version.
- If there is an external URL available, add "Open external location"
"View in Renderer" should be available only for files for which we have some renderer to use. ATM it is AFAIK only for .md files (but doesn't do anything yet)

dereklu888 commented 3 years ago

I implemented a 3dot menu for each row, with tags for opening the folder and external link -- should be up on the datasets-dev. Quick question, still mulling about rendering, do you think the renderer should open the file in the current window (like replacing what is rendered where README.md usually is), or should it lead to an external link with just the rendered object (like viewing a file on github)? For the latter, I think it's doable without further infrastructure stuff/routing (which would make things less lean), but it might be less elegant (probably like another query parameter, if we're staying with the current setup).

Also, I implemented the context menu natively instead (let me know if there's any styling issues or bugs encountered), so I won't need to host new files for it.

codecov-commenter commented 3 years ago

Codecov Report

Merging #27 (70c49eb) into master (425a8d0) will increase coverage by 3.65%. The diff coverage is 94.90%.

:exclamation: Current head 70c49eb differs from pull request most recent head 7bcb7d1. Consider uploading reports for the commit 7bcb7d1 to get more accurate results

@@            Coverage Diff             @@
##           master      #27      +/-   ##
==========================================
+ Coverage   84.43%   88.08%   +3.65%     
==========================================
  Files          13       15       +2     
  Lines        1246     1913     +667     
==========================================
+ Hits         1052     1685     +633     
- Misses        194      228      +34

Impacted Files	Coverage Δ
datalad_deprecated/__init__.py	`100.00% <ø> (ø)`
datalad_deprecated/publish.py	`87.85% <87.85%> (ø)`
datalad_deprecated/tests/test_publish.py	`100.00% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 425a8d0...7bcb7d1. Read the comment docs.

yarikoptic commented 3 years ago

re styling:

Let's

remove grey background (could be done here even though unrelated) behind that "[Open folder]", and make it in the color of the folder (of func to the left)
context menu text: make consistent in font style (may be it is) and size (seems bigger) to the items in the table itself
black text for context menu for some reason throws me off, not sure why ;)

Re renderers: let's first just add links to the remote resource like Bioimagesuite ones for .nii(.gz)?$ files listed on https://github.com/dandi/dandiarchive/wiki/WiP:-dump-of-thoughts-on-%22services-registry%22 . Those, since regular links, could be opened by user in a new window or not as they like... although may be later we will provide a uniform new target so people could sequentially open different files in that viewer without breeding tabs - yet to decide. ATM going https://bioimagesuiteweb.github.io/webapp/viewer.html?image=https://datasets-dev.datalad.org/dbic/QA/sub-emmet/ses-20180531/anat/sub-emmet_ses-20180531_acq-MPRAGE_T1w.nii.gz isn't working for me, but I will see if I just need to tune up CORS or report to bioimagesuite folks

yarikoptic commented 3 years ago

woohoo -- I have fixed web server setup to provide certificate for datasets-dev so it could be served over https, and https://bioimagesuiteweb.github.io/webapp/viewer.html?image=https://datasets-dev.datalad.org/dbic/QA/sub-emmet/ses-20180531/anat/sub-emmet_ses-20180531_acq-MPRAGE_T1w.nii.gz works for me in brave browser (on chromium site whines "Your browser does not support WEBGL (even v1). We can not proceeed.")

yarikoptic commented 3 years ago

May be right away add that MetaCell/NWBExplorer link for .nwb files. I have made a sample file available under -dev in http://datasets-dev.datalad.org/?dir=/dandi/dandisets/000027/sub-RAT123 . Although ATM display there doesn't quite work (https://github.com/MetaCell/nwb-explorer/issues/261) but I hope it would get resolved soon.

dereklu888 commented 3 years ago

I added render options for the .nii/.nii.gz and .nwb files, also tweaked the styling as per your changes (though the font for the context menu I just changed to a dark gray, if that's enough).

I'll probably implement some testing as well, but after that is there anything else on the rendering side I should work on before moving on to whichever issue is next? Maybe adding the option to swap out the README.md file that is being rendered for another .md file?

yarikoptic commented 3 years ago

also styling wise, a minor comment

if three dots could be aligned to text better (didn't check if font style/size differs from the text?) would be nice. Now it is bigger and a bit higher than text.

dereklu888 commented 3 years ago

As for the styling -- sorry, I thought you meant the font styling of the menu options themselves, not the 3 dots. I'll try to finish that up next.

dereklu888 commented 3 years ago

Alright, I updated the formatting of the 3dot menu, added an External Section separator in the menu, and added the rendering function -- let me know if the format of the rendering data looks good, I'm not sure yet if I should keep the extensions and use them as a key in a map, let me know your thoughts.

dereklu888 commented 3 years ago

For styling, I opted to make the External Services separator the same left indent as the copy/other operations, and increased the indent of the renderers (I just set it as a hard 15px). I added a few tests in (though they are currently fitted to the 3 rendering options available, might need to go update this when new rendering options are added?).

yarikoptic commented 3 years ago

Thank you @dereklu888 ! Let's proceed with this one. I will also deploy current state of the web ui to the main instance

datalad / datalad-deprecated

Adding context menu and rendering options for certain files #27

Codecov Report