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

Enable file browser for dependencies in pipeline node properties #881

Merged
merged 10 commits into from
Aug 24, 2020

Conversation

vabarbosa
Copy link
Contributor

@vabarbosa vabarbosa commented Aug 19, 2020

This PR add support for selecting file dependencies using using a BrowseFileDialog from the Properties dialog of the Pipeline Editor.

Fixes #873

  • Both files and directories are selectable
  • Multiple files and directories can be selected at a time
  • File Dependecies textarea can still be manually updated/edited
  • BrowseFileDialog opens at the notebook path and prevents navigating to parents/grandparents directories (subdirectories are navigable)
  • Selected files and/or directories are appended (relative to the notebook's parent directory) to the File Dependecies textarea

elyra-dependencies-2

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.

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.
Add support for selecting file dependencies using using a file browser  from the node properties in the Pipeline Editor.

Fixes elyra-ai#873

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
@ajbozarth
Copy link
Member

I'm still reviewing and testing, but I found a couple UI issues I figured I'd share now

The folder icon is missing from the crumbs when in a subfolder, but is still visible when in "root"
Screen Shot 2020-08-19 at 12 08 09 PM

The Add Dependancies... button overlaps withe the text box
Screen Shot 2020-08-19 at 12 08 42 PM

both issues are visible in your gif above

@vabarbosa
Copy link
Contributor Author

vabarbosa commented Aug 19, 2020

I'm still reviewing and testing, but I found a couple UI issues I figured I'd share now

The folder icon is missing from the crumbs when in a subfolder, but is still visible when in "root"

The Add Dependancies... button overlaps withe the text box

both issues are visible in your gif above

thanks, i have resolved the button alignment.

as far as the folder icon, that is the link back to JupyterLab's working directory. if a notebook is within a subdirectory then my understanding is the user should not be allowed to navigate to JupyterLab's working directory (hence the reason i hid it). in any case, i have updated the PR and took a different approach. rather hide/remove the directories, the directories are just disabled and not allowed to be clicked and traversed into:

image

not sure which approach is preferred so feedback is welcomed.

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.

A few clarifying questions, but nothing blocking. Overall this works great and looks good

@@ -506,9 +506,12 @@ export class PipelineEditor extends React.Component<
}

propertiesActionHandler(id: string, appData: any, data: any): void {
const propertyId = { name: data.parameter_ref };
const filename = this.propertiesController.getPropertyValue('filename');
Copy link
Member

Choose a reason for hiding this comment

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

Once #861 is merged this will need to be updated to

Suggested change
const filename = this.propertiesController.getPropertyValue('filename');
const filename = PipelineService.getWorkspaceRelativeNodePath(
this.widgetContext.path,
this.propertiesController.getPropertyValue('filename')
);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@vabarbosa vabarbosa requested a review from ajbozarth August 20, 2020 02:31
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.

LGTM with the caveat of the required fix if #861 gets merged

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.

Tested this locally, looks great!

Copy link
Member

@marthacryan marthacryan left a comment

Choose a reason for hiding this comment

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

Just tried locally and LGTM!

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.

Tried locally with the update to support relative paths and it still LGTM

@akchinSTC akchinSTC added this to the 1.1.0 milestone Aug 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow for updating File Dependencies using a file browser
6 participants