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(website): improve layout of sequence details page #1761

Merged
merged 3 commits into from
May 6, 2024

Conversation

chaoran-chen
Copy link
Member

@chaoran-chen chaoran-chen commented May 5, 2024

resolves #1699

preview URL: https://sequence-details.loculus.org

Summary

This improves the look of the sequence details page by:

Screenshot

600px
1200px

1200px

1900px

1900px

PR Checklist

  • [ ] All necessary documentation has been adapted.
  • [ ] The implemented feature is covered by an appropriate test.

@chaoran-chen chaoran-chen added the preview Triggers a deployment to argocd label May 5, 2024
@chaoran-chen chaoran-chen added this to the MVP milestone May 5, 2024
Copy link
Contributor

@anna-parker anna-parker left a comment

Choose a reason for hiding this comment

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

I absolutely love this! I think its starting to look really professional!
A just have a couple small layout nits:

  • I find having an author's subsection a bit odd now that we add author's at the top - additionally this leaves a lot of white space. Can we add the author's association to the sample information?
    image
  • In some previous iterations of this PR you added the scientific sequence name in italics, I think this was generally very well liked and it would be cool to still have it:
image - In this same preview you also added the orchid ID which is awesome - do you have that in another PR? Would hate to see that excellent work lost :-)

Copy link
Member

@theosanderson theosanderson left a comment

Choose a reason for hiding this comment

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

Looks really pretty

@corneliusroemer
Copy link
Contributor

aligning the labels to the left

This is undoing Theo's conscious decision to make labels right-align to make tables more readable by reducing whitespace between label and what it's labelling. I thought this was a neat solution to a tricky problem. Using fixed width causes issues as we either a) need to use a hard limit for width of labels, b) get multi-column due to lengthy labels or c) have lots of white space in case of length labels.

Re @anna-parker:

In this same preview you also added the orchid ID which is awesome - do you have that in another PR? Would hate to see that excellent work lost :-)

This was a dummy/mock feature that lacks the underlying information to be possible with current page. We need to first collect ORCID and figure out how to send it to website before it's possible to show. (Also, it's orcid without h 🙈 )

I find having an author's subsection a bit odd now that we add author's at the top - additionally this leaves a lot of white space. Can we add the author's association to the sample information?

The way we divide things into sections is very preliminary and not something for this PR to fix, generally, however, Similar to orcid, it would be nice if we could show affiliations for each author.
I agree that it might be nice to show affiliation below authors as it's also done this way for papers. But could be done in follow-up/we could make an issue.

@theosanderson
Copy link
Member

I think we should keep trying to iterate on the alignment question in a later PR. I agree with Chaoran that the simple right align I had previously would look messy here (but also agree with Cornelius on whitespace). Once this is merged we can play with if there are any other approaches to formatting these tables.

@chaoran-chen
Copy link
Member Author

Thanks for the feedback, everyone! I created two follow-up issues: #1767 and #1769. For ORCID, we already have #1616.

@chaoran-chen chaoran-chen merged commit c0bd326 into main May 6, 2024
13 checks passed
@chaoran-chen chaoran-chen deleted the sequence-details branch May 6, 2024 13:07
@chaoran-chen chaoran-chen mentioned this pull request May 6, 2024
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
preview Triggers a deployment to argocd
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Add an expand option for long sequence lists [...]
4 participants