search

ebi-webcomponents / nightingale

Data visualisation web components for the life sciences.
https://ebi-webcomponents.github.io/nightingale
MIT License
118 stars 38 forks source link

Sequence heatmap implementation #273

Closed marcelo-q closed 4 months ago

netlify[bot] commented 4 months ago

Deploy Preview for cozy-marzipan-abb3b4 ready!

Name Link
Latest commit a0896f645f18314e2c2c646cdc6f6ba9e2c257e2
Latest deploy log https://app.netlify.com/sites/cozy-marzipan-abb3b4/deploys/664311fb2ab5640008cabb4f
Deploy Preview https://deploy-preview-273--cozy-marzipan-abb3b4.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 site configuration.

gustavo-salazar commented 4 months ago

@marcelo-q the tests are failing, but from what I can see, it's just missing the snapshot files, do you mind including those?

marcelo-q commented 4 months ago

hi @gustavo-salazar, thanks for the review.

sorry about that, just added the snapshot files generated by test now

gustavo-salazar commented 4 months ago

hi @gustavo-salazar, thanks for the review.

sorry about that, just added the snapshot files generated by test now

Hey no prob, unfortunately it is still failing, I think there is still one more snaphsot missing

marcelo-q commented 4 months ago

I just removed a not needed test file that was placeholder before we renamed the lib. Maybe that was the fix

marcelo-q commented 4 months ago

Hey @marcelo-q, thanks so much for this PR.

Everything works as expected. You can take most my comments as suggestions. The only one that really needs to be changed before merging is the support of multiple highlight segments.

As an extra thing, can you add the component to the Manager story? that way we can see it playing along with other Nightingale components. And will also test if the withResizable is doing what is expected of it.

Another comment is if you had a look to the colored sequence component. It seems to me that the functionality is quite similar, with the major difference being that this one is implemented on SVG and yours is on canvas. Which in itself is reason enough to have another component. I'm just wondering if that's the only reason to create a new component instead of reusing the one that was there.

Hi @gustavo-salazar thank you very much for taking your time to review this

I've changed my code according to your comments so it fits the Nightingale lifecycle better (a bit embarrassed I did not do this before haha). If you have spotted anything else feel free to add more comments and I will be happy to try to solve it.

I also added the component to the Manager story.

Lastly I believe our main reasoning for doing this is that we are generating heatmaps with very large amounts of data in our different pages. There were some performance issues with SVG before that prompted Sameer and Misi (previous line manager) to push for adopting canvas based visualisations in our group.

To be honest Sreenath and Sameer would be able to update you better on this. Maybe we can start a more detailed discussion on slack

gustavo-salazar commented 4 months ago

Hi @gustavo-salazar thank you very much for taking your time to review this

I've changed my code according to your comments so it fits the Nightingale lifecycle better (a bit embarrassed I did not do this before haha). If you have spotted anything else feel free to add more comments and I will be happy to try to solve it.

Awesome. Just have a quick look and all seems to be OK.

I also added the component to the Manager story.

I was going to tell you that you forgot to import the component in the story. However, the story works well without it, and on a quick test it doesn't seem to need all the imports that we have at the top of that file, which then makes me wonder, how is that working, where are those elements being defined? is it something storybook does? anyways not your problem, but if you have any clue about it please share,

Lastly I believe our main reasoning for doing this is that we are generating heatmaps with very large amounts of data in our different pages. There were some performance issues with SVG before that prompted Sameer and Misi (previous line manager) to push for adopting canvas based visualisations in our group.

To be honest Sreenath and Sameer would be able to update you better on this. Maybe we can start a more detailed discussion on slack

Yeah, very similar reasoning to why we used canvas on the multiple alignment component. No need at the moment to have a more detailed conversation, as I was just curious. But good to know who to ask about it.

gustavo-salazar commented 4 months ago

I just approved this PR. I will give it a few days in case any of the UniProt fellas have the time to do a second review. Otherwise, I'll merge and publish the components next week, maybe together with the variation track, and hopefully with a dependencies update!

gustavo-salazar commented 4 months ago

I'm merging now. But will only publish the components next week, together with the variation ones