Skip to content
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

feat(app): Add Neurosift viewer for EDF and NWB files #3187

Merged
merged 2 commits into from
Nov 4, 2024
Merged

Conversation

nellh
Copy link
Contributor

@nellh nellh commented Oct 31, 2024

Implementation of #3186 using the iframe approach.

Screenshot From 2024-10-31 10-22-40

@nellh nellh mentioned this pull request Oct 31, 2024
Copy link

codecov bot commented Oct 31, 2024

Codecov Report

Attention: Patch coverage is 93.54839% with 2 lines in your changes missing coverage. Please review.

Project coverage is 44.44%. Comparing base (1c1e0d3) to head (149a8ea).
Report is 3 commits behind head on master.

Files with missing lines Patch % Lines
...app/src/scripts/dataset/files/file-viewer-type.jsx 60.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3187      +/-   ##
==========================================
+ Coverage   43.10%   44.44%   +1.34%     
==========================================
  Files         547      592      +45     
  Lines       36178    37861    +1683     
  Branches     1128     1131       +3     
==========================================
+ Hits        15595    16829    +1234     
- Misses      20383    20832     +449     
  Partials      200      200              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@magland
Copy link

magland commented Oct 31, 2024

Thanks @nellh

This won't work as is for NWB files. The URL for those looks a bit different:

https://neurosift.app/?p=/nwb&url=https://api.dandiarchive.org/api/assets/027567cf-e9e0-4aba-83c0-ef2d22c8c1b7/download/

Notice the p=/nwb instead of p=/edf

@magland
Copy link

magland commented Oct 31, 2024

I wonder what's the best way to select the height. Is the plan to just fix it as something sensible?

@nellh
Copy link
Contributor Author

nellh commented Oct 31, 2024

Thanks @nellh

This won't work as is for NWB files. The URL for those looks a bit different:

https://neurosift.app/?p=/nwb&url=https://api.dandiarchive.org/api/assets/027567cf-e9e0-4aba-83c0-ef2d22c8c1b7/download/

Notice the p=/nwb instead of p=/edf

Thanks, fixed!

I wonder what's the best way to select the height. Is the plan to just fix it as something sensible?

I set it to 50% viewport height as this is what our HTML iframe viewer was doing already and generally gives a good amount of space for the viewer without using a static height.

@nellh nellh merged commit 0d7bdd7 into master Nov 4, 2024
15 checks passed
@nellh nellh deleted the edf-viewer branch November 4, 2024 20:20
@magland
Copy link

magland commented Nov 6, 2024

This is great, thanks @nellh . When will this go live?

@nellh
Copy link
Contributor Author

nellh commented Nov 7, 2024

This is great, thanks @nellh . When will this go live?

It will be included in the next OpenNeuro release, aiming to release early next week.

@magland
Copy link

magland commented Nov 19, 2024

@nellh I think there's a problem here. It seems that when you visit the page of an EDF file, the entire file gets automatically downloaded to the browser before the iframe is rendered. This is probably because that's how other file types are handled. But in this case, we don't need the file to be downloaded - and that's the point of neurosift - that's it's lazy-loading. Sometimes the files can be quite large.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants