Cancel selection by clicking background, and hide action panel #833
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
… manually This has enough downsides that it's looking like a failed experiment. Overall, my impression is now more strongly that we should use RF for rendering only, rather than utilising its internal state. At which point, we may be able to get away with a much simpler tree/graph-rendering library. Benefits: - Original motivation: allows us to click on the background to cancel selection. - IDs can no longer pass `isNan`, because such nodes have `selectable: false`. - Slight increase in responsiveness (in theory), since initial selection change is frontend-only. - Opens a door to easy multi-selection. - Slightly simplifies `PrimerNode` type def. Drawbacks/todo: - The `deepEqual` check stops the obvious infinite loops, but feels like a hack, and I've now noticed some major flickering during edits, where selection goes on and off several times before settling. - The use of `useNodesState` is taken from examples, and crucial to this working, but docs say not to use it in production (without really saying why, or what to do instead). - Selecting multiple nodes is now possible (with shift-click and drag). We may not want this. And if we do, we shouldn't be ignoring most of them with `p.onNodeClick(p.nodes[0])`. - Our `assertType` call is getting complex as we use more and more fields from `RFNode`. - Several uses of `// @ts-ignore` - use `as` and encapsulate within `ReactFlowSafe`? - Do we really want to tie ourselves closer to RF anyway? Given that we have some concerns about performance, and other recent work has been done in the opposite direction.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
As mentioned in the commit message, I no longer think the first commit here is a good idea.
But it does at least give us an easy way to cancel a node selection by clicking the background, enabling the disappearing-action-panel behaviour in the second commit*, so I've pushed this branch to allow us to get a feel for that. Although as it happens, I'm not sure that's such a good idea now either. We proposed it during last week's in-person meeting (along with a major redesign of the sidebar), but as @dhess anticipated in yesterday's wrap-up, it actually ends of feeling a bit weird, at least when implemented as naively as this, particularly because a newly-selected node can disappear behind the panel. Further UX exploration is needed.
* Strictly speaking, it's kind-of unrelated, but without this change the panel would only not be present when a new program is created, or when the selected node is deleted.