-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
feat(grid): add an ability to drop elements from outside of a grid #980
Conversation
@n1ghtmare @josesaranda @mankinchi @jeremymlane hi! You said here #985 you want to help the project. So could you review this PR pls? And you can mention me in your PRs, I'll review them too |
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.
That is ok! Great!
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.
I installed and tested your code. It's ok for me! Congrats!
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.
Awesome feature, great job, really. I hope my comments are helpful. Let me know if you need my help.
- add isDroppable flag for grids - add onDrop callback for getting an element position after dropping - add documentation - add an example
@n1ghtmare sorry I'm new to review code on github. Can you show me how? |
@mankinchi Sure thing. First look at the top of this pull request. You will see several tabs, the first one being "Conversation" which is the one we're on now. The next one is "Commits", click on it and select the commit that you want to review (in this case it's just one). You'll now see the commit and the changes. Go through them (through the actual code) and leave a comment on any line that you feel needs a comment/clarification/suggestion or whatever. You might not have any comments of course. Once you're done, in the upper right corner you'll see "Review Changes". Click it and select the option you need. Approve accordingly. I hope this helps. Let me know if you need more clarifications. |
@n1ghtmare thanks. I am able to fetch and checkout this PR branch but couldn't get the server to run. I'm using |
Ok figured out the reason why. I think this is out of the scope of this task but not sure whether we can address it here. The build-example is pumping 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.
I think the code is quite nice. However, I'm having problem seeing onDragStart & onDragStop is being used when you drop the droppableItem. I mean it make senses to do it, but I think we would want to have a separate callback for it.
Thank you for your comment! But I think it's ok to use the same method for doing the same thing. This code uses onDragStart for dragging simulation, that's why we need to reuse it here |
@STRML check it pls. If it's ok, I'll merge it |
|
||
if (!this.currentNode) { | ||
// eslint-disable-next-line react/no-find-dom-node | ||
this.currentNode = ((ReactDOM.findDOMNode(this): any): HTMLElement); |
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.
Any chance this won't exist?
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.
@STRML no, I think it's impossible
this.currentNode = ((ReactDOM.findDOMNode(this): any): HTMLElement); | ||
} | ||
|
||
if (!dragging) { |
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.
Would be nice to refactor this out of CWRP
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.
@@ -0,0 +1,134 @@ | |||
import React from "react"; |
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.
This should be numbered 15
, not 16
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.
See comments above - forgot to hit the "submit review" button :( |
will this be pushed to npm soon or it requires the fixes for @STRML's comment first? |
Hi I think you should update @types/react-grid-layout too, since is at 0.16.7 version and doesn't have any drop related typings. |
I don't maintain that package.
…On Wed, Nov 27, 2019, 11:41 AM Paolo Bernardini ***@***.***> wrote:
Hi I think you should update @types/react-grid-layout too, since is at
0.16.7 version and doesn't have any drop related typings.
Thank for this feature.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#980>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAJEKPZ5INWITEZSDEMR3NLQV2PMBANCNFSM4IL4PX6Q>
.
|
How can I do this?? |
Hi, |
Anyway you can combine widgets into one widget with tabs? |
Relates to: #586