fix issue on failed drag makes the whole grid unresponsive #2672
Conversation
| return n?.grid && (strict ? | ||
| (n.sizeToContent === true || (n.grid.opts.sizeToContent === true && n.sizeToContent === undefined)) : | ||
| (!!n.sizeToContent || (n.grid.opts.sizeToContent && n.sizeToContent !== false))); |
There was a problem hiding this comment.
when I run the test script, it fails, and this is the result when I run yarn test --fix
There was a problem hiding this comment.
that's just reformatting (yarn lint --fix) not yarn test.... humm.
There was a problem hiding this comment.
You didn't provide an example showing the drag failing so can't reproduce. hard to tell if this is the right fix without digging into it more.
There was a problem hiding this comment.
Apologies for any confusion, I realize I forgot to include instructions on how to reproduce the issue. However, I want to clarify that this is not the source of the bug; it's related to other changes. Thank you for your understanding!
src/gridstack.ts
Outdated
| delete node.grid._isTemp; | ||
| } | ||
| delete node.grid._isTemp; |
There was a problem hiding this comment.
To reproduce the issue:
- Enable dragging a widget from outside onto the grid.
- Ensure the grid is crowded, leaving very little space (less than the size of the widget).
- Make sure the grid has defined rows, preventing movement.
- Drag the widget into that small space.
This will cause the addition of the widget to fail and become unresponsive. If you need further assistance, feel free to reach out. Thank you!
There was a problem hiding this comment.
so this doesn't fix dropping into a full grid ? what does it fix then ?
There was a problem hiding this comment.
These changes should resolve the issue. The step provided is intended to demonstrate how to reproduce the issue before implementing these changes. I apologize if the explanation is unclear.
There was a problem hiding this comment.
The issue arises because 'node.grid' is populated only when the widget is added (line 2253). Therefore, if the widget fails to be added, an error occurs because the system attempts to delete 'node.grid._isTemp', which does not exist.
I'll aim to create a reproduction of the issue sometime next week, if that works for you.
There was a problem hiding this comment.
if that's the case delete node.grid?._isTemp; woudl have suffice. I'll need to debug this to see the implication of move it out...
There was a problem hiding this comment.
| The issue arises because 'node.grid' is populated only when the widget is added (line 2253)
no it could come from another grid as well... anyway the optional null check is safe and right thing to do, but I may need to exit sooner and not do some work, but for now that fixes the js error. thank you for the demo...
|
Hi @mustafamilyas and @adumesny, does this PR also fix issue #2667 by any chance? |
|
@sfragis nothing to do with it. |
|
@adumesny, apologies for the delay. I've made the adjustments as per your suggestion and tested them on my end. It seems to have resolved the issue at least on my side. Below, you'll find a JSFiddle and a video demonstrating how to reproduce the error before the changes were made. Let me know if you need further assistance. Thank you! https://jsfiddle.net/redmaze/jqw25fed/7/ |
|
that demo is weird. items are places 1x1 first, then 2x1 (to match actual dragged size, what it should be) even if you don't resize the dropped item. |
Description
when we drag a widget and fail, it will throw the above error and make all the whole grid unresponsive
Checklist
yarn test)How to reproduce