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/graph view widget #155

Merged
merged 22 commits into from
Oct 28, 2024
Merged

Feat/graph view widget #155

merged 22 commits into from
Oct 28, 2024

Conversation

Pooya-Oladazimi
Copy link
Collaborator

This is a basic implementation for the graph view. I could not transfer one feature for removing a node from my implementation in Tib TS since there I did not use useQuery which creates an issue for this feature. The rest is there though.

@Pooya-Oladazimi Pooya-Oladazimi linked an issue Oct 18, 2024 that may be closed by this pull request
src/app/types.ts Outdated
Copy link
Collaborator Author

@Pooya-Oladazimi Pooya-Oladazimi Oct 21, 2024

Choose a reason for hiding this comment

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

I'm sorry about this. My new code formatter automatically formatted this TS file, which is the reason behind these large changes.

@jusa3
Copy link
Collaborator

jusa3 commented Oct 22, 2024

Very nice implementation, it's fun to play around with the moving badges.
I noticed the following (not all of these may be important and need to be fixed in the prototype):

  • The root walk is only a parameter in the storybook. Would it be nice to have it as a button next to the reset button?
  • Would be nice to have the loading spinner a bit more central or bigger beside the icon - I didn't see it at first and wondered why nothing happened. Same with the error message
  • If I have a long path to the root (e.g. 12 nodes), I can't get them positioned properly in the graph view window. They sort of arrange themselves into a big curve that doesn't fit into the graph view window box. But I guess that's a limitation of the design library and the tree view is better for this kind of use case.
    image
  • For EFO diabetes mellitus and the SemLookP API:
    testLabel instead of diabetes mellitus (happens when switching between rootWalk true and false, corrects itself when clicking reset)
    image
    walk is wrong: a thing is not an endocrine disease
    image
  • For EFO diabetes mellitus and the EBI API, thing is not present.
    image
  • If part of the graph (I think the current entity) is outside the window, resetting sometimes causes the graph to disappear (you have to scroll out to find it again). Perhaps resetting could also centre the current entity.
  • Water story for Chebi is missing (http://purl.obolibrary.org/obo/CHEBI_15377)
  • Help menu items have different indentations
    Screenshot from 2024-10-22 11-11-48

@Pooya-Oladazimi
Copy link
Collaborator Author

@jusa3

Thanks for the extensive review. I forgot to make this pr a draft since it is still in dev due to the newly requested features.

  • Yes, one of the concern is that the graph becomes a long snake if the root is far away from the target node. I need to check how I can play around with the graph config to make it take less space.
  • The rest of the points are not finished as I need to differentiate between the normal graph call and the ancestor call to find the walk path from the root.

@Pooya-Oladazimi Pooya-Oladazimi marked this pull request as draft October 22, 2024 10:22
@Pooya-Oladazimi Pooya-Oladazimi marked this pull request as ready for review October 23, 2024 11:34
@Pooya-Oladazimi
Copy link
Collaborator Author

@rombaum @jusa3

This is ready for testing and review.

@jusa3
Copy link
Collaborator

jusa3 commented Oct 23, 2024

Very nice and quick implementation :)

Some feedback (on general usability):

  • Colouring the current entity differently to improve visibility (especially in root walk) could be a nice feature. Perhaps the use of colours for different relationships in general.
  • The information/instructions on how to use the graph could be a bit more styled. Maybe as a EuiPopover?
  • As a user, I would be a bit overloaded with a long root walk. It looks rather messy to me. The filter for different relationships might solve that.
    image

@Pooya-Oladazimi
Copy link
Collaborator Author

@jusa3

Thanks. The first two points, I agree. I will implement those.
Regarding, the third part, all the relations in the root walk are the same (is-a) and therefore not possible to filter.

@jusa3
Copy link
Collaborator

jusa3 commented Oct 24, 2024

Haha, you're right about the third point and the "water" example. In this case, I think that this type of graph does a good job of representing the complex relationships of water.

@Pooya-Oladazimi
Copy link
Collaborator Author

@jusa3

Changes have been implemented.

Copy link
Collaborator

@jusa3 jusa3 left a comment

Choose a reason for hiding this comment

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

Just a small spelling error. Otherwise ready to go.

I also removed the graph view issue that would have been closed by this PR. Some of the CHEBI requirements mentioned there aren't implemented yet.

src/components/widgets/GraphViewWidget/GraphViewWidget.tsx Outdated Show resolved Hide resolved
@Pooya-Oladazimi Pooya-Oladazimi merged commit 1c7ffe9 into main Oct 28, 2024
2 checks passed
@Pooya-Oladazimi Pooya-Oladazimi deleted the feat/graph-view-widget branch October 28, 2024 13:36
@jusa3
Copy link
Collaborator

jusa3 commented Oct 28, 2024

🎉 This PR is included in version 2.12.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@jusa3 jusa3 added the released label Oct 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants