refactor: rename draggingElement -> newElement
#8294
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Skipped Deployment
|
dwelle
changed the title
draggingElement -> newElement
dwelle
dismissed
their stale review
we actually need to make sure that old dragged elements aren't being set as newElement and used as such
| (local.id === localAppState.editingElement?.id || | ||
| local.id === localAppState.resizingElement?.id || | ||
| local.id === localAppState.draggingElement?.id || // TODO: Is this still valid? As draggingElement is selection element, which is never part of the elements array | ||
| local.id === localAppState.newElement?.id || // TODO: Is this still valid? As newElement is selection element, which is never part of the elements array |
There was a problem hiding this comment.
@Mrazator should we also prefer local element if it's being dragged? Not that we can detect it efficiently right now as we don't track dragged elements
There was a problem hiding this comment.
This part is potentially incorrect. If we want to preserve local element that's being dragged, we should update the logic. Otherwise, we should just remove the changed line of code.
There was a problem hiding this comment.
should we also prefer local element if it's being dragged
probably, though now thinking that these conditions could momentarily lead to an inconsistent reconciliation result on different clients; though such cases will immediately be fixed with the next update, so it is likely not necessarily a big issue
There was a problem hiding this comment.
yeah, though it's incorrect in production as well. Now at least we prefer elements that are being currently created, though it likely doesn't matter and it's unlikely those will be updated on remote (unless there's a bug in sync).
If we want to prefer local dragged elements, we'd likely need to introduce a new state for all dragged elements (map preferably, for perf).
| } else if (points.length > 1 && isElbowArrow(newElement)) { | ||
| mutateElbowArrow( | ||
| newElement, | ||
| this.scene, | ||
| [...points.slice(0, -1), [dx, dy]], | ||
| [0, 0], | ||
| undefined, | ||
| { | ||
| isDragging: true, | ||
| }, | ||
| ); |
There was a problem hiding this comment.
this was removed by mistake.
Case: updating elbow arrow points as you create it.
There was a problem hiding this comment.
a test would be nice. Maybe just checking if points are middle points are being set/updated when creating "2-point" elbow arrow?
| } else if ( | ||
| !this.state.newElement && | ||
| !this.state.selectedElementsAreBeingDragged && | ||
| !this.state.selectionElement | ||
| ) { |
There was a problem hiding this comment.
we shouldn't reset snapLines when we're dragging (the "when selecting" is just a perf optim as right now it's a noop)
If not checked, the snap lines flicker as they're constantly being set and reset
| if ( | ||
| !this.state.newElement && | ||
| !this.state.selectionElement && | ||
| !this.state.selectedElementsAreBeingDragged && | ||
| !this.state.multiElement | ||
| ) { |
There was a problem hiding this comment.
this doesn't have any effect right now, but if we later have a :hover effect when over scrollbars or something, it would matter. So adding the checks to keep parity with prod.
There was a problem hiding this comment.
let's not write test for this. It's actaully just half the story, and scrollbars are broken anywa.
| !this.state.newElement && | ||
| !this.state.selectionElement && | ||
| !this.state.selectedElementsAreBeingDragged |
There was a problem hiding this comment.
parity with prod: don't support toolbar shortcuts while selecting or dragging
| ) && | ||
| (this.state.selectionElement || | ||
| this.state.newElement || | ||
| this.state.selectedElementsAreBeingDragged || |
There was a problem hiding this comment.
parity with prod: we need to disable UI pointer events when dragging elements over UI
There was a problem hiding this comment.
let's write a test for this by checking for existence of --ui-pointerEvents on some UI
| !appState.newElement && | ||
| !appState.selectedElementsAreBeingDragged && | ||
| !appState.selectionElement |
There was a problem hiding this comment.
let's write a test checking that undo/redo is disabled while dragging/selecting
|
@ryan-di todos:
|
| /** | ||
| * currently set for: | ||
| * - text elements while in wysiwyg | ||
| * - newly created linear elements while in line editor (not set for existing | ||
| * elements in line editor) | ||
| * - and new images while being placed on canvas | ||
| */ |
There was a problem hiding this comment.
added docs for editingElement. I'd say this one is ripe for refactor as well as it doesn't make much sense.
draggingElementis a misnomer and implies it's related to elements being dragged, which is mostly false.newElementis a more appropriate name and wheredraggingElementwas used as described above has been replaced withselectedElementsAreBeingDragged