-
Notifications
You must be signed in to change notification settings - Fork 350
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
Create a dropzone component for dragging and dropping jupyter files into canvas #1062
Conversation
Thanks for making a pull request to Elyra! To try out this branch on binder, follow this link: |
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.
LGTM
Thanks @bourdakos1, overall it looks good to me. One question I have is about the actual dropzone, is this something specific to pipelines? Would code-snippet, which is enabling some darg-drop support be able to reuse this? This could help us decide overall where the location of this component should reside. |
@lresende I wouldn't say this is specific to pipelines. However, I'm guessing this won't be useful for code-snippets, because this dropzone component only handles the "drop". Where code-snippets handles "dragging from", and I assume the Notebook itself handles the actual "drop". If we have another widget where we are dragging and dropping something onto the widget itself, it would be useful there. |
In general this look good to me, I don't fully understand the details in some of the new files, but I assume it's just leveraging react more directly. I do have a few meta comments though:
I'll take some time to check this out and test locally later today as well |
tested locally and it works great |
@ajbozarth I’m just used to a react components being a folder. The index file is the react component, I can definitely move it to ui-components I called it LuminoDropzone because the events it listens for are from “@lumino/dragdrop” |
That makes sense, I figured it was a react related reason. Prior to starting dev on lab I preferred creating subduers like this anyway.
Thanks, I'll "approve" once you've move it
gotcha |
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.
Tried locally and it worked as expected. Code LGTM. Thanks!
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.
Also, if using a directory, could we name it maybe dragdrop
as in Lumino
or dropzone
?
Create a dropzone component for dragging and dropping files (notebooks, python scripts, etc) into the pipeline editor canvas
Notes:
eslint-plugin-react-hooks
for checking react hooksreact/prop-types
because we use TypeScript and it seems like we don't use PropTypesDeveloper's Certificate of Origin 1.1