Conversation
…/reactFlow/components/ReactFlowCanvas.tsx
This reverts commit 24586e8.
molly-moen
left a comment
molly-moen
left a comment
There was a problem hiding this comment.
This looks really cool! Couple small comments, not blocking.
| ); | ||
| return; | ||
| } | ||
|
|
There was a problem hiding this comment.
We'll probably want to do some refactoring of this file to split it up a bit, but that should be a follow up. I think we'll hit some (hopefully easily resolvable) conflicts with my open pr: https://github.com/code-dot-org/code-dot-org/pull/72285/changes#diff-ace9cf822f2f2ed4aa8449d365844fc5133b457594967265eb153fd690d0574e
There was a problem hiding this comment.
👍 agreed, it's getting hefty.
|
|
||
| // Arrow keys on a focused line edge move the whole line by moving | ||
| // both line-anchor nodes together. | ||
| if (!readOnly && focusedEdgeId) { |
There was a problem hiding this comment.
it seems like there's a bit of duplicate logic between the two if statements, can we extract anything out here?
There was a problem hiding this comment.
Good call, let me see if I can dedupe this a bit.
| SketchlabReactFlowNode, | ||
| } from '@cdo/apps/lab2/types'; | ||
|
|
||
| export function canCreateConnection( |
There was a problem hiding this comment.
worth adding a comment here? I think it's checking that you aren't connecting a 'line' to an existing node, is that right?
There was a problem hiding this comment.
Sorry missed this, simplified and added a comment here: ca2f28f
Yeah hopefully clearer now, but it's just there to prevent other nodes from connecting to a (ghost) line anchor.
| announce(`Edge created to ${getNodeLabel(targetNode)}.`); | ||
| if (connectionRejected) { | ||
| announce( | ||
| 'Connection not created. Line endpoints may only have one edge.' |
There was a problem hiding this comment.
I think if I got this message as a user I would be confused. What does it mean in terms of what the user sees on the screen?
There was a problem hiding this comment.
Yeah good callout, I had questions about this as well. This is for the case where you're in the "connect mode" via keyboard nav, and try to connect to one of the "nodes" of a line.
I just tested it with a screenreader, and it did correctly reject a connection. We might want to do something else in the future (make line nodes non-tabbable when in connect mode?), but for now I can add a clearer message.
2 participants
Implements a line as an edge that connects two "ghost nodes".
As an alternative, I went down the path of implementing this as a "custom node" (very rough): https://github.com/code-dot-org/code-dot-org/compare/ben/sketch2-line
I think implementing it as an edge makes sense longer term because we'll be able tap into the built-in goodness that React Flow includes for "lines" as part of its edge implementation.
Their docs (linked below) also mention the idea ("What about an “edge” not connected to any nodes at all?") of an edge with no nodes (ie, what we're trying to do), so that seems like a reasonable standard approach.
line.mov
Links
Testing story
Tested manually (see video above).
Follow-up work