fix: Add binding update to manual stat changes #8183
Conversation
Signed-off-by: Mark Tolmacs <mark@lazycat.hu>
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Ignored Deployment
|
|
What about the other direction, when it is the arrow (bound to some elements) that we're updating through stats panel? Should we update the bindings? For instance, the arrow is moved to a different location
|
|
@dwelle Should we update the arrow bindings if manually update via the stats panel? It would make sense that if you move an arrow close to a bindable it would bind. |
Signed-off-by: Mark Tolmacs <mark@lazycat.hu>
Signed-off-by: Mark Tolmacs <mark@lazycat.hu>
|
I merged in the master branch and added in functionality to keep linear elements bound if they were previously bound and the manual changes don't remove them from their original binding area. Otherwise the linear element disconnects. Previously it never disconnected the original binding, no matter how far it was moved via stats edits. Consequently it never bound to any shape either when moved within the binding area. |
yeah, sounds good! |
|
Okay, if @ryan-di if you're okay with this, no more edge cases come to mind, then give me the go-ahead and I merge this. |
…ioning Signed-off-by: Mark Tolmacs <mark@lazycat.hu>
Signed-off-by: Mark Tolmacs <mark@lazycat.hu>
Signed-off-by: Mark Tolmacs <mark@lazycat.hu>
Signed-off-by: Mark Tolmacs <mark@lazycat.hu>
Signed-off-by: Mark Tolmacs <mark@lazycat.hu>
|
@ryan-di added tests and fixed a small bug. Let me know. |
Signed-off-by: Mark Tolmacs <mark@lazycat.hu>
Signed-off-by: Mark Tolmacs <mark@lazycat.hu>
Signed-off-by: Mark Tolmacs <mark@lazycat.hu>
|
Removed import reordering as per @dwelle's request. |
|
Hey @mtolmacs, I've gone through the code and if you could take some time to go over the two comments, we're good to go 🙂 |
| interface AngleProps { | ||
| element: ExcalidrawElement; | ||
| elementsMap: ElementsMap; | ||
| scene: Scene; |
There was a problem hiding this comment.
What's the purpose of using scene here?
| ): boolean => { | ||
| const threshold = maxBindingGap(element, element.width, element.height); | ||
| const shape = app.getElementShape(element); | ||
| const shape = getElementShape(element, elementsMap); |
There was a problem hiding this comment.
Is the inclusion of the getElementShape refactor intentional?
There was a problem hiding this comment.
Unfortunately yes, because the getElementShape() was an instance method on App and the binding code uses getElementShape(), therefore an app instance must be provided to the linear binding functions used this PR. Since it was a fairly self-contained instance method, I opted to extract it from App and not add an app instance to all functions all the way down to the stats editor. I also bring this refactor in another PR, so this is a refactor which should happen anyway.
I believe the code and responsibilities are clearer this way.
| angle: nextAngle, | ||
| }); | ||
|
|
||
| if (isLinearElement(latestElement)) { |
There was a problem hiding this comment.
I would probably put this in a function in Stats/utilts 🙂
Signed-off-by: Mark Tolmacs <mark@lazycat.hu>
| ): boolean => { | ||
| const threshold = maxBindingGap(element, element.width, element.height); | ||
| const shape = app.getElementShape(element); | ||
| const shape = getElementShape(element, elementsMap); |
There was a problem hiding this comment.
Unfortunately yes, because the getElementShape() was an instance method on App and the binding code uses getElementShape(), therefore an app instance must be provided to the linear binding functions used this PR. Since it was a fairly self-contained instance method, I opted to extract it from App and not add an app instance to all functions all the way down to the stats editor. I also bring this refactor in another PR, so this is a refactor which should happen anyway.
I believe the code and responsibilities are clearer this way.
| angle: nextAngle, | ||
| }); | ||
|
|
||
| if (isLinearElement(latestElement)) { |
Manual stats changes now respect previous element bindings.
Currently manual stats updates do not respect already bound linear elements.