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

Legend and Relations revised, few cleanups #60

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

Conversation

tmushayahama
Copy link
Contributor

Issues

#59
#50
#49

Changes

@pkalita-lbl can you verify the scss changes, idk how to handle the new logistics, but wanted to make the legend take the full width, so hacked by adding a bunch of parent classes to retain the already defined classes. Lemme if acceptable. Also my formatter was acting up, so it didn't preserve the newlines sometimes

cc @vanaukenk @kltm

Copy link
Collaborator

@pkalita-lbl pkalita-lbl left a comment

Choose a reason for hiding this comment

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

Overall looks good! A few suggestions about styling, CSS Parts, and CSS Properties.

src/components/gocam-legend/gocam-legend.scss Outdated Show resolved Hide resolved
src/components/gocam-legend/gocam-legend.tsx Outdated Show resolved Hide resolved
src/components/gocam-legend/gocam-legend.tsx Outdated Show resolved Hide resolved
src/components/gocam-legend/gocam-legend.tsx Outdated Show resolved Hide resolved
src/components/gocam-viz/gocam-viz.tsx Outdated Show resolved Hide resolved
src/components/gocam-viz/gocam-viz.tsx Outdated Show resolved Hide resolved
@tmushayahama
Copy link
Contributor Author

@pkalita-lbl I have made the changes. Thanks for checking this out

Comment on lines 125 to 129
.gocam-container {
height: 100%;
display: flex;
flex-direction: column;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't see this class used anywhere. Please remove.

@pkalita-lbl
Copy link
Collaborator

Looking pretty good now. Still a few loose ends to tie up:

  • I think a few unnecessary things can still be removed from gocam-viz.scss, unless I'm misunderstanding your comment about the width of the legend.
  • The CSS Part elements within the legend need the part attribute on them.

NOT RIGHT NOW: we should think about whether to move what you've added as dbxref.service.ts to its own standalone NPM package (or, even better, update the existing @geneontology/dbxrefs package).

@pkalita-lbl
Copy link
Collaborator

Sorry! One more thing I just noticed. The legend graphic for provides input for shows a circle then an arrow. In the actual graph the edge has an arrow then a circle:

image

While I'm being nit-picky, is it possible to adjust the alignment of the arrow and line here? It sort of looks like the line is continuing all the way through the arrow. Maybe that's a matter of adjusting the refX attribute?

image

@tmushayahama
Copy link
Contributor Author

Thanks for pointing the legend graphics, I have made the fix, The triangle so pointy now and no longer allowed as a carry-on luggage :) . I might try to reduce the code to use same marker, I ended having negative viewBox

Copy link
Collaborator

@pkalita-lbl pkalita-lbl left a comment

Choose a reason for hiding this comment

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

Awesome! Thanks for addressing all the feedback. Feel free to merge whenever you're ready.

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.

2 participants