Lattice-Automation / seqviz

a JavaScript DNA, RNA, and protein sequence viewer
https://tools.latticeautomation.com/seqviz
MIT License
247 stars 53 forks source link

Checking that there is indeed a selection before scrolling #257

Closed guzmanvig closed 9 months ago

guzmanvig commented 10 months ago

Same as before but now I check that the start and end the selection are different. It seems that when you click on some row, the selection prop gets populated with the index you clicked on in start and end

jjti commented 9 months ago

@guzmanvig what is the intended behavior? I'm not sure what's happening now is desirable, clicking on any element, even if it's already in full view of the Linear Viewer, scrolls the Linear viewer so that element is at the top of the viewer. I prefer it the way it is now without that auto-scrolling behavior

guzmanvig commented 9 months ago

@guzmanvig what is the intended behavior? I'm not sure what's happening now is desirable, clicking on any element, even if it's already in full view of the Linear Viewer, scrolls the Linear viewer so that element is at the top of the viewer. I prefer it the way it is now without that auto-scrolling behavior

Screencast.from.02-13-2024.11.51.37.AM.webm

@jjti The intended behavior is for when the user programmatically sets a selection. In our application, the user clicks on a list of indices, which when clicked, they are selected in Seqviz. However, currently, the selection may not be visible and the user needs to manually scroll Seqviz to find it. This would fix that as it would automatically scroll to whatever is selected.

Would you want me to apply this logic only for programatic selection and not for when the user clicks? Or should I check that the selection is not visible before scrolling? Not sure if the latter is possible

jjti commented 9 months ago

Would you want me to apply this logic only for programatic selection and not for when the user clicks?

I think this is the way to go imo. Selections set from clicks on elements differ from the selection that users pass in. They have more fields like type (which is not publicly documented): https://github.com/Lattice-Automation/seqviz/blob/beb9455d0f659545837eae3a862fe1e14b2e9dea/src/selectionContext.ts#L16

So maybe check whether that .type field is set and only update state if it is?

guzmanvig commented 9 months ago

@jjti your solution worked, but with one caveat. The selection prop type was just { clockwise?: boolean; end: number; start: number; }, so the type field didn't exist in it. I added an | Selection to the type to fix this, and then a type guard to be able to check for the type field. What I couldn't figure out was why selecting by clicking updates the props. Shouldn't it just update the state? Maybe that's something that's wrong somewhere else, and we need to fix that instead of doing this. Let me know and I can look into it.