Update node properties to allow changing file #867

vabarbosa · 2020-08-17T14:22:19Z

This PR adds support for changing the file associated with a node in the Pipeline Editor.

Fixes #732

To achieve this a new BrowseFileDialog was added into @elyra/ui-components. The BrowseFileDialog displays a file browser in a dialog window which resolves to the selected files when accepted.

It is worth noting JupyterLab already contains a FileDialog in @jupyterlab/filebrowser. However, this does not allow flexibility and customization easily, hence the reason for creating the new BrowseFileDialog. Unlike the FileDialog, the BrowseFileDialog can be configured to allow for disabling/preventing multiselect and also allow for selecting directories (if needed). The FileDialog does includes the New Folder, Upload, and Refresh buttons (that appear in the default FileBrowser) but not options to opt out of displaying those buttons.

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.

Added a file browser dialog and added support for changing file from the properties dialog Fixes elyra-ai#732

kevin-bates · 2020-08-17T14:44:42Z

@vabarbosa - this looks great! Just curious if the same could be done for the File Dependencies section (where the start point is the location of the notebook file)? If so, then that would eliminate folks typing in names/paths to existing files. I'd be happy to open the issue if that's something we want to do.

vabarbosa · 2020-08-17T14:53:35Z

@vabarbosa - this looks great! Just curious if the same could be done for the File Dependencies section (where the start point is the location of the notebook file)? If so, then that would eliminate folks typing in names/paths to existing files. I'd be happy to open the issue if that's something we want to do.

@kevin-bates you read my mind, i was thinking the same thing as was going to bring it up for discussion during the scrum to see if it is something would like to have included

lresende · 2020-08-17T14:56:26Z

This looks awesome!!!

marthacryan · 2020-08-17T16:48:18Z

This is so cool!! Just one style thing - for some reason this is showing up a little misaligned for me (in chrome)

I didn't investigate a ton but it looks like it's being split in half between the filename and the button - any ideas?

And in safari it's the same:

vabarbosa · 2020-08-17T17:02:00Z

This is so cool!! Just one style thing - for some reason this is showing up a little misaligned for me (in chrome)
I didn't investigate a ton but it looks like it's being split in half between the filename and the button - any ideas?
And in safari it's the same:

@marthacryan i've updated the button position so it is consistent across the browsers. the 50/50 split comes canvas. i am using the columnPanel provided by canvas to display/group multiple items in the same row:

https://github.com/elyra-ai/canvas/wiki/3.3-Common-Properties-Controls#grouppanel-controls

marthacryan · 2020-08-17T17:29:31Z

Just tried your new changes and style is working as expected! One (very) small thing - definitely doesn't need to be part of this PR, but it'd be great if you could select a file in that browser by double-clicking. Not sure how easy that'd be to add, just a note.

marthacryan

LGTM

marthacryan

Sorry - just noticed that there's a console error that should probably be fixed before merging:

vabarbosa · 2020-08-17T17:59:22Z

@marthacryan i used a different panel layout and worked around the 50/50 split. yes, i noticed the console errors when switching to chrome. looking into it now. and will look into enabling double-click to select also. thanks

vabarbosa · 2020-08-17T20:33:47Z

Sorry - just noticed that there's a console error that should probably be fixed before merging:

@marthacryan i believe the error is coming from the carbon components modal (used by common canvas to display the properties dialog). more specifically, carbon components modal does not support stacked modals:

carbon-design-system/carbon#5947
carbon-design-system/carbon#582

in any case, i was able to figure out a workaround the issue and the error should no longer be thrown.

vabarbosa · 2020-08-17T21:02:18Z

Just tried your new changes and style is working as expected! One (very) small thing - definitely doesn't need to be part of this PR, but it'd be great if you could select a file in that browser by double-clicking. Not sure how easy that'd be to add, just a note.

@marthacryan, you should now be able to select a file by double-clicking

marthacryan · 2020-08-17T21:27:18Z

@vabarbosa Thanks for addressing those! Tried with new changes locally and LGTM

packages/pipeline-editor/src/PipelineEditorWidget.tsx

lresende

Tested locally with chrome, LGTM.

ajbozarth

still need to test locally and review the BrowseFileDialog file, but wanted to get these early comments in

packages/pipeline-editor/src/PipelineEditorWidget.tsx

ajbozarth · 2020-08-17T22:39:22Z

packages/pipeline-editor/src/PipelineEditorWidget.tsx

@@ -456,6 +464,13 @@ export class PipelineEditor extends React.Component<
    }
    const app_data = node.app_data;

+    if (app_data.filename !== propertySet.filename) {
+      app_data.filename = propertySet.filename;


As a note: this line will need to be updated to work with #861 is merged (in whichever PR is merged last)

packages/pipeline-editor/src/utils.ts

…file

ajbozarth

One clarifying question, otherwise I've checked this out and run it locally and it works great. Once my previous review is addressed I'll approve

ajbozarth · 2020-08-17T22:50:36Z

packages/pipeline-editor/src/PipelineEditorWidget.tsx

+      const propertyId = { name: data.parameter_ref };
+      showBrowseFileDialog(this.browserFactory.defaultBrowser.model.manager, {
+        filter: (model: any): boolean => {
+          return model.type == 'notebook';


Is the intension here that we would update this when we add support for other file types?

yes, the filter is so you only show files that should be selectable. in this case, we only want to show notebook files (no point in showing all available files if user shouldn't be selecting them). and yeah, if the dialog is to be re-used in other context then you can configure what files do appear. for example, with File Dependencies (#873) browser dialog would show files other than just notebook files

ajbozarth · 2020-08-17T23:10:53Z

packages/pipeline-editor/src/PipelineEditorWidget.tsx

+    if (app_data.filename !== propertySet.filename) {
+      app_data.filename = propertySet.filename;


Note this would specifically need to be updated to the following once #861 is merged or in #861 once this is merged.

Suggested change

if (app_data.filename !== propertySet.filename) {

app_data.filename = propertySet.filename;

const propertyFilename = PipelineService.getPipelineRelativeNodePath(

this.widgetContext.path,

propertySet.filename

);

if (app_data.filename !== propertyFilename) {

app_data.filename = propertyFilename;

ajbozarth

LGTM

kevin-bates

Good stuff Va - thank you!

Update node properties to allow changing file

cadd6ab

Added a file browser dialog and added support for changing file from the properties dialog Fixes elyra-ai#732

vabarbosa requested review from lresende, marthacryan, ajbozarth and karlaspuldaro August 17, 2020 14:22

vabarbosa self-assigned this Aug 17, 2020

Update button for consistency across browser

d9c78c1

Update browse button css

c77d5a2

marthacryan approved these changes Aug 17, 2020

View reviewed changes

marthacryan requested changes Aug 17, 2020

View reviewed changes

Improve browse button layout

abab7e7

Resolve maximum call stack error

9d843ad

Enable select file with doubleclick

44794a1

vabarbosa requested a review from marthacryan August 17, 2020 21:02

marthacryan approved these changes Aug 17, 2020

View reviewed changes

vabarbosa mentioned this pull request Aug 17, 2020

Allow for updating File Dependencies using a file browser #873

Closed

kevin-bates reviewed Aug 17, 2020

View reviewed changes

packages/pipeline-editor/src/PipelineEditorWidget.tsx Outdated Show resolved Hide resolved

lresende approved these changes Aug 17, 2020

View reviewed changes

Move get filename to util function

24ec481

vabarbosa requested a review from kevin-bates August 17, 2020 22:43

ajbozarth requested changes Aug 17, 2020

View reviewed changes

Merge remote-tracking branch 'upstream/master' into vabarbosa-browse-…

359796e

…file

ajbozarth reviewed Aug 17, 2020

View reviewed changes

address review comments

4dfe3be

vabarbosa requested a review from ajbozarth August 17, 2020 23:08

ajbozarth reviewed Aug 17, 2020

View reviewed changes

ajbozarth approved these changes Aug 17, 2020

View reviewed changes

kevin-bates approved these changes Aug 17, 2020

View reviewed changes

lresende merged commit d5ac043 into elyra-ai:master Aug 18, 2020

ajbozarth mentioned this pull request Aug 18, 2020

Update pipeline editor to use relative paths #861

Merged

lresende mentioned this pull request Oct 17, 2020

Use Elyra UI node label as KFP node name #971

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update node properties to allow changing file #867

Update node properties to allow changing file #867

vabarbosa commented Aug 17, 2020 •

edited

Loading

kevin-bates commented Aug 17, 2020

vabarbosa commented Aug 17, 2020

lresende commented Aug 17, 2020

marthacryan commented Aug 17, 2020 •

edited

Loading

vabarbosa commented Aug 17, 2020 •

edited

Loading

marthacryan commented Aug 17, 2020

marthacryan left a comment

marthacryan left a comment

vabarbosa commented Aug 17, 2020

vabarbosa commented Aug 17, 2020

vabarbosa commented Aug 17, 2020

marthacryan commented Aug 17, 2020 •

edited

Loading

lresende left a comment

ajbozarth left a comment

ajbozarth Aug 17, 2020

ajbozarth left a comment

ajbozarth Aug 17, 2020

vabarbosa Aug 17, 2020 •

edited

Loading

ajbozarth Aug 17, 2020

ajbozarth left a comment

kevin-bates left a comment

		if (app_data.filename !== propertySet.filename) {
		app_data.filename = propertySet.filename;

-    if (app_data.filename !== propertySet.filename) {
-      app_data.filename = propertySet.filename;
+    const propertyFilename = PipelineService.getPipelineRelativeNodePath(
+      this.widgetContext.path,
+      propertySet.filename
+    );
+    if (app_data.filename !== propertyFilename) {
+      app_data.filename = propertyFilename;

Update node properties to allow changing file #867

Update node properties to allow changing file #867

Conversation

vabarbosa commented Aug 17, 2020 • edited Loading

kevin-bates commented Aug 17, 2020

vabarbosa commented Aug 17, 2020

lresende commented Aug 17, 2020

marthacryan commented Aug 17, 2020 • edited Loading

vabarbosa commented Aug 17, 2020 • edited Loading

marthacryan commented Aug 17, 2020

marthacryan left a comment

Choose a reason for hiding this comment

marthacryan left a comment

Choose a reason for hiding this comment

vabarbosa commented Aug 17, 2020

vabarbosa commented Aug 17, 2020

vabarbosa commented Aug 17, 2020

marthacryan commented Aug 17, 2020 • edited Loading

lresende left a comment

Choose a reason for hiding this comment

ajbozarth left a comment

Choose a reason for hiding this comment

ajbozarth Aug 17, 2020

Choose a reason for hiding this comment

ajbozarth left a comment

Choose a reason for hiding this comment

ajbozarth Aug 17, 2020

Choose a reason for hiding this comment

vabarbosa Aug 17, 2020 • edited Loading

Choose a reason for hiding this comment

ajbozarth Aug 17, 2020

Choose a reason for hiding this comment

ajbozarth left a comment

Choose a reason for hiding this comment

kevin-bates left a comment

Choose a reason for hiding this comment

vabarbosa commented Aug 17, 2020 •

edited

Loading

marthacryan commented Aug 17, 2020 •

edited

Loading

vabarbosa commented Aug 17, 2020 •

edited

Loading

marthacryan commented Aug 17, 2020 •

edited

Loading

vabarbosa Aug 17, 2020 •

edited

Loading