Skip to content
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

Add files to pipelines through file browser context menus #882

Merged
merged 4 commits into from
Aug 24, 2020

Conversation

marthacryan
Copy link
Member

Fixes #817. Shows a new option in the context menu of notebook files.
image

Developer's Certificate of Origin 1.1

   By making a contribution to this project, I certify that:

   (a) The contribution was created in whole or in part by me and I
       have the right to submit it under the Apache License 2.0; or

   (b) The contribution is based upon previous work that, to the best
       of my knowledge, is covered under an appropriate open source
       license and I have the right under that license to submit that
       work with modifications, whether created in whole or in part
       by me, under the same open source license (unless I am
       permitted to submit under a different license), as indicated
       in the file; or

   (c) The contribution was provided directly to me by some other
       person who certified (a), (b) or (c) and I have not modified
       it.

   (d) I understand and agree that this project and the contribution
       are public and that a record of the contribution (including all
       personal information I submit with it, including my sign-off) is
       maintained indefinitely and may be redistributed consistent with
       this project or the open source license(s) involved.

@marthacryan marthacryan added status:Work in Progress Development in progress. A PR tagged with this label is not review ready unless stated otherwise. and removed status:Work in Progress Development in progress. A PR tagged with this label is not review ready unless stated otherwise. labels Aug 19, 2020
Copy link
Member

@ajbozarth ajbozarth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other than one nit this LGTM, I ran it locally and this is great. I do have a handful of gripes I'll share below, but this deals with them as best as can be already.

  • I don't like having to pass the signal up and down between the extension and widget, but even though we could move the code into the component class to minimize this it would mean every instance of a pipeline editor would try adding the command, which would probably not work as expected.

  • I feel like we should distinguish the item in the context menu with a better icon, but any icon other than + wouldn't be a good fit for the action it performs, so + will have to work.

  • I'd rather have the context menu only show if the editor is open, but that may be too difficult to implement and there already another context menu item that is also present even when inapplicable and behaves the same way as this (Shut Down Kernel).

Copy link
Member

@karlaspuldaro karlaspuldaro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree with Alex's points noted, but I think this is a great enhancement as is for now :)

@marthacryan marthacryan requested a review from ajbozarth August 20, 2020 18:45
Co-authored-by: Alex Bozarth <ajbozart@us.ibm.com>
@marthacryan marthacryan requested a review from ajbozarth August 21, 2020 15:21
@akchinSTC akchinSTC added this to the 1.1.0 milestone Aug 21, 2020
@lresende lresende merged this pull request into elyra-ai:master Aug 24, 2020
@marthacryan marthacryan deleted the context-menu branch April 23, 2021 15:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make adding a node to a pipeline accessible
5 participants