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

Add tooltip w/ visx customization to volcano plots #381

Merged
merged 11 commits into from
Aug 15, 2023

Conversation

jernestmyers
Copy link
Contributor

@jernestmyers jernestmyers commented Jul 25, 2023

Resolves #66

This PR imports our changes to the @visx/xychart in place of airbnb/visx's latest version. I did so as follows:

  "@visx/xychart": "https://github.com/jernestmyers/visx.git#visx-xychart",

Testing

This work can be tested in our normal workflow, by:
  1. Checking out this branch (66-volcano-plot-tooltips)
  2. Running yarn at the root of the monorepo
  3. Spinning up a local mbio-site dev server (and skipping the cache just to be safe) via:
  yarn nx start @veupathdb/mbio-site --skip-nx-cache


.VolcanoPlotTooltip > ul > li {
font-weight: normal;
}
Copy link
Contributor Author

@jernestmyers jernestmyers Jul 31, 2023

Choose a reason for hiding this comment

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

NOTE: I'm just glancing back over this and can't see why this would be necessary. Perhaps I'm misremembering something. If not, I'll remove when I address feedback!

UPDATE: Seems to be necessary, one way or another, to set font-weight: normal. Otherwise, all the content is bold. Will keep it as it is.

@jernestmyers
Copy link
Contributor Author

Here's a screencast demonstrating the styles and the logic that combines pointIDs into a single tooltip when the coordinates are shared.

Note how the grey background uses black text whereas the other background colors use white text. This is to achieve an acceptable color ratio. We may want to render the background colors at the default opacity, which may be more reflective of the marker color as I'm unsure how many users will adjust the marker opacity anyway. Doing so may also necessitate changes to the text colors.

Screen.Recording.2023-08-09.at.12.12.40.PM.mov

@dmfalke
Copy link
Member

dmfalke commented Aug 9, 2023

Regarding building our code with your changes to visx, here is an idea:

In your fork, set up a workflow that builds the code and commits it to a branch named something like dist. In our repo, we can specify your fork and the branch for the visx package. See https://yarnpkg.com/cli/add.

I'm not sure if we will have to do this for all visx packages.

I'll try to find an example workflow that does what I suggested.

@jernestmyers
Copy link
Contributor Author

NOTE: the directions for testing have been edited. We no longer need to do anything special to test this now that I've deployed the visx/xychart code into a branch on my fork and pointed our components package.json to that branch.

@jernestmyers jernestmyers marked this pull request as ready for review August 14, 2023 20:59
Copy link
Member

@dmfalke dmfalke left a comment

Choose a reason for hiding this comment

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

I think this looks great!

@jernestmyers jernestmyers merged commit d67dec7 into main Aug 15, 2023
1 check passed
@jernestmyers jernestmyers deleted the 66-volcano-plot-tooltips branch August 15, 2023 18:07
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.

Volcano: add tooltips
2 participants