[WIP] Select cells tip #66
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
| // This currently removes the tracks visually, but the cells are still selected. Is this intentional? | ||
| clearTracks={() => { | ||
| setSelectedPoints(undefined); | ||
| return canvas?.removeAllTracks(); |
There was a problem hiding this comment.
This was being returned previously, I don't think it needs to be, but I left it while debugging.
The bigger issue is that without this addition, clicking "Clear tracks" does not reset the selectedPoints, so the state of the left panel does not reset. With this change, the state resets, but some of the tracks continue to get drawn and then there is no 'Clear Tracks' button to clear them.
There was a problem hiding this comment.
Maybe this is the culprit? https://github.com/aganders3/points-web-viewer/blob/main/src/components/App.tsx#L170
There was a problem hiding this comment.
I agree this doesn't need to actually return anything - I think just the compact form was making it return something automatically. In any case I think the result of the function it's returning removeAllTracks is also undefined so I'm not sure it makes a difference.
On the bigger issue - this is something we have yet to clear up with selection, and basically hits on one of the larger outstanding decisions we need to make. See also #61.
selectedPoints is at least partly just bad naming. It's is a transient value that is set by the selection helper, used to trigger an effect that fetches the corresponding tracks (and their ancestors/descendants) and adds them to the canvas. It's re-set whenever a user makes a new selection, even though the previously loaded tracks remain. It's extra confusing because points correspond to cells within a timeframe, but a track could also be considered a single cell (multiple points) as it moves through time.
edit: selectedPoints should maybe even be re-set to empty (and I think at some point was) after sending requests for all the corresponding tracks.
All this to say: if you look at #64 we're using currently loaded number of tracks to indicate whether a selection has been made.
One final thought - my impression from @clarsen-czi's designs was that this info pane would only show on first launch, and disappear forever once any selection had been made. If this is the case I think all of the above is actually irrelevant to the current PR. I will defer to design on how they'd like that to actually work (I don't think Connor reads these github notifications so we can just ask in Slack).
There was a problem hiding this comment.
Agreed with the summary from Ashley that selection is a mess, so anything that touches it is likely to get burned. Given the input from design, I think this change is fine for now. Though we probably want to avoid future tweaks to anything selection related until we get a proper handle on it.
There was a problem hiding this comment.
Thanks for all of this context!
Would checking the currently loaded number of tracks mean something like canvas.tracks.size > 0?
There was a problem hiding this comment.
The callout is useful, and I like code reorganization here too. I'll defer to @aganders3 to work out how best to integrate this with #64.
|
Ah yeah I screwed this up by eagerly merging #64 (and then the actual PR #69 🤦). @ehoops-cz I'm happy to do the work to fix the merge conflicts if you'd like. |
Resolves #65
Add a callout to tell the user how to select cells. This PR shows the callout whenever no cells are selected.
Some open questions are in code comments.
