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

Replace jekyll site with zarr_samples.csv #20

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

will-moore
Copy link
Member

@will-moore will-moore commented Feb 16, 2023

With the app at https://ome.github.io/ome-zarr-catalog we can largely replace the Jekyll site.

This repo can still be used for simply listing the IDR ngff samples. This PR replaces the whole repo with a single csv file,
containing only the data that isn't loaded dynamically from the Zarr image, and a home page that hosts the catalog in an iframe.

To test, see page at https://will-moore.github.io/ome-ngff-samples/

Copy link
Member

@sbesson sbesson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Based on the set of coupled PRs, I understand the URLs that should compared are https://deploy-preview-2--deft-vacherin-6c652f.netlify.app/?csv=https://raw.githubusercontent.com/will-moore/ome-ngff-samples/replace_page_with_csv/zarr_samples.csv&columns=Version,Axes,shape,chunks,Wells,Fields,Keywords,Thumbnail vs https://idr.github.io/ome-ngff-samples/.

Based on the above, a few immediate comments:

  • the URL shortening looks stangre
  • the icon of the validator is now the mainline OME icon. Is that expected?
  • the ability to sort columns and to control the pagination are lost, so is the search box
  • the column order is different. Although there was no strict order, in terms of usability, the thumbnail is buried at the far right of my browser window and the DOI column which is mostly blank is right at the center
  • some of the keywords e.g. labels seem to be lost, are these meant to be turned into separate columns?
  • the link to the source IDR data in the Study column is lost

There might be some discussion on each of these point to assess whether they are blockers or not. I have not been part of the discussion behind the migration to the new catalog framework so I cannot comment on the expectations. The review above assumes the target catalog should be a like-for-like replacement but some of the changes might be on purpose and up for evaluations.

In terms of review process, I assume adding new entries could be tested more simply by opening a PR against this repository and providing a URL to the catalog endpoint using the staging CSV as the input? If so, that's definitely an improvement compared to the current workflow and something we'll want to add e.g. to the README.

README.md Outdated Show resolved Hide resolved
@will-moore
Copy link
Member Author

@sbesson - thanks for the review - yes, a few features aren't quite ready yet, so I think this PR can wait until we feel that the new page is good enough to replace the old.

  • The URL shortening simply uses CSS to add ellipses when the link gets too long for the space available. Advantages are that it's declarative (doesn't require and JavaScript) and handles any URL, since we don't know anything about the structure of URLs that will be provided. But this also means that it will truncate at "random" places in the URL. The only alternative I can think of is we give users some way to provide link text (e.g. support markdown where they can add custom links and hide the 'URL' column). But this gets more complicated to configure etc.
  • Unfortunately the ome-ngff-validator doesn't have it's own icon, but it uses the ome icon as it's favicon for the page. I also think that this is really the OME way of viewing an NGFF image, so until the validator has it's own icon, the OME icon feels like an improvement over the grey check-mark?
  • Sorting & search: I could see these being features that we add in follow-up PRs but they're not in the "MVP". The current PR is not "release-ready" but is getting big enough to be mergeable. Search and sort should be easy-enough to do with the csv data, but harder for the dynamically loaded zarr data since there's no table of that data. I wasn't sure whether search and sorting were important features of the existing table, or if they were just "freebies" that came with the table layout. Created issue Table sorting ome/ome-zarr-catalog#6
  • The column order can mostly be dynamic, based on the order of the columns in the CSV which will always come before the dynamic zarr columns. So, moving the DOI to the end isn't currently doable, but moving Thumbnail to the first of the "zarr columns" is supported: https://deploy-preview-2--deft-vacherin-6c652f.netlify.app/?csv=https://raw.githubusercontent.com/will-moore/ome-ngff-samples/replace_page_with_csv/zarr_samples.csv&columns=Thumbnail,Version,Axes,shape,chunks,Wells,Fields,Keywords Markdown support would also allow a shorter link text to DOI links, which would help the layout.
  • Keywords - yes, sorry I just noticed that "plate" keyword isn't being shown - I did have that working - will check... I guess I could also dynamically check for labels too... The other keyword that we have is coordinateTransformation but this is quite a subjective feature (not likely that most users will want to highlight the existence of that?) so that could maybe go in the csv data. Or maybe we move all the keywords into csv so we have more control over them?
  • IDR links - again, this needs markdown support. Created issue at Support markdown in csv table ome/ome-zarr-catalog#5

@will-moore will-moore force-pushed the replace_page_with_csv branch from 3c80f8f to a52e184 Compare February 16, 2023 15:16
@will-moore
Copy link
Member Author

@sbesson Instead of getting rid of the jekyll site and add a redirect, I decided instead to use an iframe to bring the ome-zarr-catalog table onto the main page.
This way, we don't have to change the URL, and can add custom header and other text to the page etc.
However, it means we still have the jekyll deployment issues in development, where I don't see the full page built correctly with $ bundle exec jekyll serve so I don't know if the iframe works with the header etc.

@joshmoore
Copy link
Member

In general, using samples as a testing ground makes sense since the new JS-based infra should minimally do everything we need. Since the testing is tricky, though, it might make sense that we set up a temporary jekyll-based repo that you can push to for comparison.

@will-moore
Copy link
Member Author

If I configure my fork of this repo correctly, can I get it to deploy a chosen branch at https://will-moore.github.io/ome-ngff-samples/ to use for testing?

@will-moore
Copy link
Member Author

OK, this branch is now deployed at https://will-moore.github.io/ome-ngff-samples/
This is now just a simple index.html page - no jekyll build required to view it locally, making it possible to view edits without deploying.

@joshmoore
Copy link
Member

joshmoore commented Feb 21, 2023

Thanks, @will-moore. First thing I noticed is that the user doesn't necessarily have any idea that things are being loaded in the background.

Also, it's expected that the thumbnails don't zoom when embedded as an iframe?

@will-moore
Copy link
Member Author

The thumbnails don't zoom as I'm not using viv and Deck.gl any more. I'm just loading pixels for lowest resolution with zarr.js and manually rendering to canvas. I got Deck.gl errors when there where more than 16 instances on the page.
I could try to replace a thumbnail with a viv/Deck.gl viewer when you click on it (maybe a bit bigger than the thumb?) - but probably not in this PR.
I'll look at adding some loading... indicator

@joshmoore
Copy link
Member

👍

@will-moore
Copy link
Member Author

@josh - Added Loading... placeholders in ome/ome-zarr-catalog#2. Test at: https://will-moore.github.io/ome-ngff-samples/

@joshmoore
Copy link
Member

👍

@will-moore
Copy link
Member Author

Updated to use https://ome.github.io/ome-zarr-catalog - and updated PR description.

@will-moore
Copy link
Member Author

@joshmoore do we still want to pursue this change to the ome-ngff-samples page?
Current version of this PR deployed at https://will-moore.github.io/ome-ngff-samples/

@joshmoore
Copy link
Member

I definitely still like the idea. What's held me back has been the lack of parity in the look&feel. Do you have the feeling that we are getting closer? (Can't do a great review from this device)

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.

3 participants