Skip to content

Conversation

@vincerubinetti
Copy link
Member

@vincerubinetti vincerubinetti commented May 13, 2025

Compare the Netlify preview here to the main one at https://molevolvr.netlify.app/testbed, where I re-enabled the Nightingale IPR viz so you could compare. Note that it seems to slow down the page, which is why I had commented it out originally while developing other components.

  • remove bespoke "install playwright" action, due to me observing it causing problems in this repo, and not being able to get it to reliably save any time
  • get rid of eslint config compatibility lines, clean up
  • update all package versions (all minor bumps)
  • delete nightingale packages
  • on CI, only run e2e tests in chromium
  • css var name changes, remove vestigial styles, misc tweaks
  • develop new interpro visualization component using d3
  • reduce testbed msa data to reduce # of elements on page at same time. in real life, msa wont be on screen at the same time as every other component we have, like it is on the testbed

@netlify
Copy link

netlify bot commented May 13, 2025

Deploy Preview for molevolvr ready!

Name Link
🔨 Latest commit d9d8a6a
🔍 Latest deploy log https://app.netlify.com/projects/molevolvr/deploys/682ccfb1a0bc000008dc01ad
😎 Deploy Preview https://deploy-preview-53--molevolvr.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

@vincerubinetti
Copy link
Member Author

Apologies for the trickling commits. This should be stable and ready to review.

@falquaddoomi
Copy link
Contributor

For the record, the old Nightingale version at https://molevolvr.netlify.app/testbed is incredibly slow for me in Chrome 136.0.7103.114 on OS X 15.4.1. It takes something like 15-30 seconds to display the page, and the IPR widget is also very laggy. I tried it without extensions enabled with the same result. I'm surprised by that since I don't remember that being the case when I last reviewed it. It's much more performant on Firefox.

@vincerubinetti
Copy link
Member Author

vincerubinetti commented May 20, 2025

Indeed, I also don't remember it being that slow. I can only think of a few possible causes for the sudden change:

  • I did a minor package bump to Nightingale, and it had a regression
  • I changed something about my use of Nightingale that they didn't intend/design for
  • Something else I added to my code (maybe another component, or one of the util funcs) that has a poor interaction with Nightingale

In any case, I wasn't happy with Nightingale for other reasons.

Copy link
Contributor

@falquaddoomi falquaddoomi left a comment

Choose a reason for hiding this comment

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

Nice work! The new widget appears to be much more performant than Nightingale; kudos. I tried it out on my Android phone, and pinch-to-zoom and drag are working as expected in the new widget.

This is a small thing, and I'll have to let the bioinformaticians weigh in on whether this is important, but I noticed in comparing it to Nightingale that it's not immediately obvious in the new widget what the full extent of the range is. It would be nice to see both the full range's start and end positions somewhere, and optionally the start and end positions of the currently visible area. It'd also be nice for the scrollbar to be closer to the position scale so you can better tell the area you're viewing, but I realize that's impractical with the native scrollbar.

This may be outside the scope of this PR, but I wonder if we should include some inline documentation on how to use the widgets, since users might have seen Nightingale before, but not this. For example, they might not know to use the mouse wheel to zoom on desktop, or that they can pan the entire sequence (rather than dragging, which might take a while) with the scrollbar at the bottom.

@vincerubinetti
Copy link
Member Author

vincerubinetti commented May 20, 2025

it's not immediately obvious in the new widget what the full extent of the range is. It would be nice to see both the full range's start and end positions somewhere, and optionally the start and end positions of the currently visible area.

Pushed a commit that always shows the start/end positions of the sequence characters in view (and when fully zoomed out, as by default, you see the sequence length).

scrollbar to be closer to the position scale so you can better tell the area you're viewing, but I realize that's impractical with the native scrollbar.

The scrollbar isn't a native one, but a "fake" one, reimplemented with d3 for better control and syncing with the global transform. I originally put it as a row right underneath the position and sequence, but then I decided it was better to put it at the bottom to mimic native browser overflow behavior. Better to stick with precedence/expectations here. Also it looked weird being closer to the top.

This may be outside the scope of this PR, but I wonder if we should include some inline documentation on how to use the widgets

For now, I've put a tooltip when hovering over the position and sequence rows. If necessary, in the future we could do something like this, where an overlay briefly shows when hovering over it the first time a user sees it:

Screenshot 2025-05-20 at 2 14 44 PM

Or just a very small legend with the same instructions permanently there. Or even better, just use the "help" component, which would look like Controls (?) with the full explanation in the (?) tooltip

Before doing that though, I'd like to see if it's necessary. I.e., do users actually have trouble with it, and do we even need users to be able to zoom in/out (1.0 doesn't have this functionality). I'd also like to wait and see if any other components end up having the same functionality (e.g. maybe in a very dense heatmap, this zoom functionality could be useful), and then implement them all in the same way.

@vincerubinetti
Copy link
Member Author

Or even better, just use the "help" component

Okay I've just done this for now. Should be sufficient for now, unless we observe users having problems.

@vincerubinetti vincerubinetti merged commit 3c899b1 into main May 20, 2025
4 checks passed
@vincerubinetti vincerubinetti deleted the interpro-redo branch May 20, 2025 18:53
@vincerubinetti vincerubinetti mentioned this pull request May 20, 2025
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.

3 participants