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: page for external ids MA-918 #574

Merged
merged 14 commits into from
Jun 2, 2021
Merged

Conversation

inghylt
Copy link
Contributor

@inghylt inghylt commented May 11, 2021

Originally a draft to get feedback, this PR now closes #575

This PR was created by @e0 and @inghylt

@mihai-sysbio mihai-sysbio changed the title feat: external ids page ma 918 feat: page for external ids MA-918 May 12, 2021
@e0 e0 linked an issue May 17, 2021 that may be closed by this pull request
@mihai-sysbio
Copy link
Member

mihai-sysbio commented May 20, 2021

This is what I'm seeing:

image

So my first thoughts are:

  • this page should follow the left/right borders of the rest of the pages (except for the Map Viewer), so I was expecting to see the title starting at the same vertical line as the logo
  • the title would be more meaningful if it's something like {{ dbname }} {{ type }} {{ id }}, where the type is the GEM component (eg reaction, metabolite), eg BiGG reaction DST_ANTIGENt
  • the word "Link" should be avoided, perhaps "go to {{ dbname }}" with a link icon
  • there is no value in showing the full link to the user, so it should be a redirect on-click
  • the models should be listed with their "original" name instead of the raw dbname, ie Rat-GEM instead of RatGem (since this is already implemented in the codebase, perhaps this can be a shared helper function)
  • the model version should also be listed
  • there should be a one-line introduction like "This {{ type }} is used in the following models:"

@e0
Copy link
Member

e0 commented May 20, 2021

Great stuff and very actionable points @mihai-sysbio !


  • the word "Link" should be avoided, perhaps "go to {{ dbname }}" with a link icon
  • there is no value in showing the full link to the user, so it should be a redirect on-click

We debated this a bit during the implementation and thought it could be useful to display the full link. If we don't show the full link at all maybe we can turn the title into a link instead.


  • the models should be listed with their "original" name instead of the raw dbname, ie Rat-GEM instead of RatGem (since this is already implemented in the codebase, perhaps this can be a shared helper function)

This could be done either in the frontend or backend. Any preferences?


  • the model version should also be listed

Sounds good. Want to add that for implementation, we currently don't include the version in the URLs for gem browser components. So for now it would be enough to list the version but for the future we should keep in mind to update this page when we start supporting multiple versions of the same model.

@mihai-sysbio
Copy link
Member

We debated this a bit during the implementation and thought it could be useful to display the full link. If we don't show the full link at all maybe we can turn the title into a link instead.

There is value in providing users with the link, but it doesn't have to be directly under the title. On the desktop, users can hover over to see the link. Otherwise they can right click or long press to copy the link. If you still think they should see it, could it be placed somewhere less "prime"?

This could be done either in the frontend or backend. Any preferences?

Could it be done entirely in the backend?

it would be enough to list the version

👍🏻

@e0
Copy link
Member

e0 commented May 25, 2021

Thanks again for the feedback @mihai-sysbio . @inghylt and I have made some updates. Please take another look when you have time. Here is a screenshot for convenience:

image

@mihai-sysbio
Copy link
Member

Overall this is really cool 👍🏻 I've left some minor feedback, not sure if I should already approve the PR.

@mihai-sysbio
Copy link
Member

Alright, to me there are the outstanding items to discuss before being ready to merge this in:

  • the term CompartmentalizedMetabolite is unknown to the user, so it should be replaced with the more familiar Metabolite
  • can we do the switch from external-db to identifier?
  • the list is visually hard to dissociate from the rest of the content (first introductory line, last line with link to identifiers.org); one way to solve this would be to bring back the list, or use some Bulma styling for lists
  • do you like the idea to make the tag hoverable as below (green border)?

In the meantime, I'll go ahead and approve this.

image

@mihai-sysbio mihai-sysbio self-requested a review May 31, 2021 11:44
@e0
Copy link
Member

e0 commented May 31, 2021

Alright, to me there are the outstanding items to discuss before being ready to merge this in:

  • the term CompartmentalizedMetabolite is unknown to the user, so it should be replaced with the more familiar Metabolite
  • can we do the switch from external-db to identifier?
  • the list is visually hard to dissociate from the rest of the content (first introductory line, last line with link to identifiers.org); one way to solve this would be to bring back the list, or use some Bulma styling for lists
  • do you like the idea to make the tag hoverable as below (green border)?

In the meantime, I'll go ahead and approve this.

Thanks for the new feedback @mihai-sysbio ! @inghylt and I have gone through them and made some changes. We thought the tags with the link styling already make it clear that they are clickable, so we decided to not implement the green hover effect.

image

Copy link
Contributor

@nanjiangshu nanjiangshu left a comment

Choose a reason for hiding this comment

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

This PR looks great to me. I also think the text of the reaction with hyperlink looks better without the green hover. I have only one comment regarding the order of GEMs
e.g. for http://localhost/identifier/BiGG/PPNCL3, it shows

Reaction  PPNCL3 from Rat-GEM 1.1.0
Reaction  PPNCL3 from Mouse-GEM 1.1.0
Reaction  PPNCL3 from Human-GEM 1.6.0

I don't know in which order are the reactions listed for different GEMs. Maybe we can order it alphabetically as done in the Explore (http://localhost/explore) page?

@mihai-sysbio
Copy link
Member

order it alphabetically as done in the Explore page?

@nanjiangshu very good observation.

Since now there are more and more places where results could/should be sorted alphabetically, how about we create an issue to revise the API and implement these were adequate? It feels like this could be a self-contained issue, without affecting the frontend at all.

@inghylt inghylt mentioned this pull request Jun 1, 2021
6 tasks
@inghylt inghylt merged commit bee191e into develop Jun 2, 2021
@inghylt inghylt deleted the feat/external-ids-page_MA-918 branch June 2, 2021 09:33
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.

page for external ids
4 participants