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

Idr front page #2

Merged
merged 25 commits into from
Mar 22, 2022
Merged

Idr front page #2

merged 25 commits into from
Mar 22, 2022

Conversation

will-moore
Copy link
Member

@will-moore will-moore commented Mar 14, 2022

Rename the module to idr_gallery and remove non IDR pages.
Also ports new IDR front page work from ome#91

Deployed at https://merge-ci.openmicroscopy.org/web/idr_gallery/

NB: that doesn't require much config as this is now the default behaviour (although the same config exists and can be overridden).
Deployment above is now:

pip install git+https://github.com/will-moore/omero-gallery.git@idr_front_page#egg=idr_gallery
omero config append omero.web.apps '"idr_gallery"'
omero config set omero.web.gallery.base_url "https://idr-testing.openmicroscopy.org/"
omero config set omero.web.gallery.gallery_index "https://idr-testing.openmicroscopy.org/"

These aren't set on idr or idr-testing, so we don't want the default to be idr-testing
@pwalczysko
Copy link

deployed locally successfully. Also checked the deployment on merge-ci.
Works fine, just a minor point

There used to be a scrollbar on the twitter feed column ? That seems to be gone now. Is it intentional ?

],
"omero.web.gallery.idr_studies_url":
["IDR_STUDIES_URL",
'https://raw.githubusercontent.com/IDR/idr.openmicroscopy.org/master/_data/studies.tsv',
Copy link
Member

Choose a reason for hiding this comment

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

There are two concerns to be reviewed before moving to production here:

  • first, although the impact is probably low, I fear we are introducing unnecessary friction and coupling between this app and the master branch of the other repository. For instance, a PR adding the statistics for a study loaded into idr-next but not released yet would need to be put on hold
  • second, as mentioned on Monday, we will want to tests what happens when this UI gets deployed and accessed externally. Is there a risk we will get a Rate Limit error?

Cross-linking to the discussion on ome#91 (comment) where a couple of options were proposed.

["INDEX_JSON_URL",
'https://raw.githubusercontent.com/will-moore/idr.openmicroscopy.org/idr_index_data/_data/idr_index.json',
str,
"URL to load JSON, with a 'tabs' list of {'title':'', 'text':'', 'videos':[]}"
Copy link
Member

@sbesson sbesson Mar 15, 2022

Choose a reason for hiding this comment

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

Similarly to #2 (comment), I am concerned about this coupling and how it works in practice. Now the app is effectively forked, is there still an incentive to loading this content from a third-party location?

Copy link
Member Author

Choose a reason for hiding this comment

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

The reason not to simply include this as HTML in the app was because updating the content would need a re-release of the app, and re-deploy etc. instead of a config change.
If that's no-longer a concern, then I can add this JSON back into the idr-gallery repo?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I brought up this issue in the former omero-gallery repository because of the number of steps (app release, configuration change) involved to upgrade a production deployment e.g. when fixing a typo.

Looking at this proposal, my concern we are shifting this burden from to the review process. For instance, a change to the content of this tab (e.g. a video update or a description update) would require a PR against idr.openmicroscopy.org, a temporary IDR/deployment PR pointing at a personal branch and a deployment of the playbook with this temporary PR to be evaluated?

Copy link
Member Author

Choose a reason for hiding this comment

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

@sbesson That commit (4ed9441) moves the tabs JSON into the idr_gallery app, instead of loading from github.

@jburel
Copy link
Member

jburel commented Mar 17, 2022

From CELL-IDR or Tissue-IDR

  • Select group by type
  • Click on the icon to view the image
  • It does not launch the viewer but redirect to cell/tissue idr

@jburel
Copy link
Member

jburel commented Mar 17, 2022

#2 (comment) has been fixed

It will look neater if the height of the twitter feed and the height of videos box match

MANIFEST.in Outdated Show resolved Hide resolved
return $("<li>")
.append("<a>" + item.label + "</a>")
.appendTo(ul);
}
Copy link
Member

Choose a reason for hiding this comment

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

add line

idr_gallery/static/idr_gallery/categories.js Show resolved Hide resolved
// Load stats and show spinning counters...

// In case studies.tsv loading from github fails, show older values
let totalImages = 12840301;
Copy link
Member

Choose a reason for hiding this comment

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

Values will need to be updated at regular interval.
Could a note be added in the instructions otherwise we won't be able to find it

@jburel
Copy link
Member

jburel commented Mar 18, 2022

The "Eye" icon on the background image does not open on idr-testing but on idr like the rest. So that should probably be configured correctly. Also I think it will be better to open that image in iviewer than the standard web view to match the other "eye" icons behaviour.

@will-moore
Copy link
Member Author

I think that the link from the background image should show the image in webclient because it provides context: If you're interested in what study etc. the Image is from and what it shows then it is more useful to see it in the webclient. And it's very easy to go from there to the full viewer if you want. It's much harder to go from iviewer back to webclient.
We discussed this in one of the meetings a while back and I think people were OK with that approach, but we could bring it up again to check?
For me, the usage of the "eye" icon really means "this is a link to show that image" and doesn't necessarily mean it has to go to iviewer. We could try a different icon, but it might be less clear that it's referring to the image? If we were going for 2 links, then we could have (i) info link and "eye" link to viewer, but I think that adds clutter without providing much benefit (since it's easy to open the viewer from webclient).

For some links to idr, I have used absolute URLs for convenience, since they will be the same once this is deployed on IDR.
But I could look at changing that...

@jburel
Copy link
Member

jburel commented Mar 18, 2022

The context is no different for the thumbnails and the background image. We have the idr accession number/name when mousing over the "circle"
Having the same icon for 2 different outcome is confusing in my view.
I do not remember that discussion (I might not have been there!)

@jburel
Copy link
Member

jburel commented Mar 18, 2022

For some links to idr, I have used absolute URLs for convenience, since they will be the same once this is deployed on IDR.

The reason I am indicating that is if we want to use a non-release background image, the link will fail if we use the "eye" mentioned above.

@will-moore
Copy link
Member Author

In the tooltip you have a link to the Experiment/Screen (which gives you context), but you don't have that for the background images. If both the tooltip thumbnail and the background image have to link to the same place, I'd prefer is if they both link to webclient/?show=image-id.
If I'm interested in the image, e.g. https://idr.openmicroscopy.org/webclient/img_detail/13965767/ tells me the name E14.lif but tells me nothing about the image, whereas https://idr.openmicroscopy.org/webclient/?show=image-13965767 I can see the Organism, and Organism Part from KV pairs and find the study. The iviewer is a dead-end, whereas the webclient encourages more browsing of data.

@jburel
Copy link
Member

jburel commented Mar 18, 2022

you do have the name and accession number when you mouse over the circle
Screenshot 2022-03-18 at 13 15 16

@jburel
Copy link
Member

jburel commented Mar 18, 2022

A point discussed with @will-moore that might confuse people (I was!)
104 studies != 70 Cell-idr, 35 Tissue-IDR!

@will-moore
Copy link
Member Author

But the accession number isn't the same as a link. You'd have to search for the study, and even then, you wouldn't be able to find that Image in the study.

@sbesson
Copy link
Member

sbesson commented Mar 18, 2022

104 studies != 70 Cell-idr, 35 Tissue-IDR!

I can understand the confusion but that reflects the fact these categories are not mutually exclusive for a submission.
Actually, in a sentence, these numbers should be read "There are 7,401,738 cell images (126 TB) across 70 studies". Does that speak for swapping the order and putting images first ?

@jburel
Copy link
Member

jburel commented Mar 18, 2022

The fact that we only have one is probably why it can be confusing.
Maybe having X images Y TB across Z studies will be better.
@will-moore what do you think?

@will-moore
Copy link
Member Author

I honestly don't think that anyone is going to visit IDR and Cell - IDR and Tissue - IDR, note the numbers, do the maths and find any problem. I also don't think that X images Y TB across Z studies solves the 35 + 70 != 104 problem because it still says that there are 35 Tissue studies and 70 Cell studies, so it doesn't explain that some studies can be both.

Also, I like 104 Studies first, as that's the primary counter of what IDR contains. And Images is nicer in the middle from a design perspective as it's the longest text looks more symmetrical.

This is less nice:
Screenshot 2022-03-18 at 14 16 25

I don't think this is an improvement:
Screenshot 2022-03-18 at 14 18 15

@jburel
Copy link
Member

jburel commented Mar 18, 2022

@will-moore thanks for the various options.

@will-moore
Copy link
Member Author

@jburel background images/links now use the 'current' server. Same for study links in tooltips. Deployed on testing server.

@sbesson
Copy link
Member

sbesson commented Mar 22, 2022

As discussed this morning, the last commit proposes to have two links associated with each image in the top-level carousel:

  • link to the viewer URL directly via the eye icon
  • link to the webclient URL via the info icon

@pwalczysko
Copy link

pwalczysko commented Mar 22, 2022

The icon on the carousel images was discussed with the team this morning and the result of the discussion was implemented - two icons, one leading to webclient, one to iviewer, see below. LGTM
Screenshot 2022-03-22 at 10 27 17

Copy link
Member

@jburel jburel left a comment

Choose a reason for hiding this comment

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

Tested the icons and it works
I assume the package will be pushed to pypi. cf README instructions

@sbesson sbesson merged commit 84fdbda into IDR:master Mar 22, 2022
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.

4 participants