-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix issue on failed drag makes the whole grid unresponsive #2672
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -111,9 +111,9 @@ export class Utils { | |
|
||
/** true if we should resize to content. strict=true when only 'sizeToContent:true' and not a number which lets user adjust */ | ||
static shouldSizeToContent(n: GridStackNode | undefined, strict = false): boolean { | ||
return n?.grid && (strict ? | ||
(n.sizeToContent === true || (n.grid.opts.sizeToContent === true && n.sizeToContent === undefined)) : | ||
(!!n.sizeToContent || (n.grid.opts.sizeToContent && n.sizeToContent !== false))); | ||
return n?.grid && (strict ? | ||
(n.sizeToContent === true || (n.grid.opts.sizeToContent === true && n.sizeToContent === undefined)) : | ||
(!!n.sizeToContent || (n.grid.opts.sizeToContent && n.sizeToContent !== false))); | ||
Comment on lines
+114
to
+116
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. when I run the test script, it fails, and this is the result when I run There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. that's just reformatting (yarn lint --fix) not yarn test.... humm. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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! |
||
} | ||
|
||
/** returns true if a and b overlap */ | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To reproduce the issue:
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so this doesn't fix dropping into a full grid ? what does it fix then ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| 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...