Replace common properties dialog with side pane #1084

marthacryan · 2020-11-18T20:45:24Z

Opens a side pane view of the node properties editor instead of a dialog.

Replace dialog with side pane
Show currently selected node's properties in the side pane (changes on single-clicking a node)
Replace broken "title editor" at top of side pane with the "label" of the node OR fix "title editor" and allow users to change the label of a given node.
EDIT No longer adding this in this PR: Add refactored code from [WIP] pipelines 2.0 #969
Clean up css to look closer to the JupyterLab appearance (i.e. save / cancel buttons, etc.)
Move descriptions of inputs from inside the text box to a question mark that can be hovered over

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.

Siteproxy

elyra-bot · 2020-11-18T20:45:25Z

Thanks for making a pull request to Elyra!

To try out this branch on binder, follow this link:

kevin-bates · 2020-11-18T21:26:33Z

I like this idea - thank you.

I think Luciano may have touched on this in the scrum regarding real estate, but I'm wondering if there's a way to introduce ellipses at a certain point to prevent the wrapping of things like Filename and even our checkbox labels. I think it would be nice to keep those to a single line but allow the full content to be visible when hovering over the ellipses or something. I've seen some extremely long (obnoxious really) file hierarchies and one of those would fill up the window if left unattended. 😄

Can this pane be "un-docked" then resized/widened?

marthacryan · 2020-11-19T22:02:21Z

@kevin-bates There'll be some CSS changes coming soon to address the ellipses comment 👍. In terms of the floating dock, it doesn't seem to be in the scope of this PR to add that, but I added an issue elyra-ai/canvas#379 and we can also look into implementing that ourselves in a future PR. However, it's worth noting that the side pane has a button that allows the user to about double the pane's width (see below):

marthacryan · 2020-12-18T19:23:36Z

@ajbozarth I'm going to work on some cleanup but here's an overview of the changes in the meantime if you want to do any ux / ui review.
Current appearance:

Expanded:

On hover for long paths (note: need feedback on this - any ideas on what I should be doing here?) :
Before hover:

While hovering:

Dropdown:

ajbozarth · 2020-12-19T00:01:32Z

I'll take a look at this once I return from vacation in 2 weeks (since this is still WIP and no rush)

ptitzler · 2020-12-21T16:29:18Z

A few general observations:

We should mark required properties. If the widget doesn't support that (yet) natively, the info text should state if a value is required.
Probably there should not be empty default entries ^^ just like there is none for dependencies
The button labels should be specific:Add item -> Add environment variable + ...
In the old UI the input text fields contained a help text, e.g. ENV_VAR=value, which was very useful
Validation still needs to be performed (e.g. empty values and invalid values (e.g. PATH should be rejected)

Inconsistency:

drag a notebook onto the canvas and define the required properties. save the changes. open the properties again.
select a different file in the properties view. save the changes. open the properties again. the file extension is gone

ptitzler · 2020-12-21T18:42:27Z

An undesired side effect that this design change introduces is that a user might unintentionally loose changes, as illustrated in this scenario:

add two nodes to a pipeline
change the properties of the first node (without clicking OK)
select the second node
select the first node - the change in the first node is lost

Previously a user was forced to either save or discard changes before another node was selected. If possible the behavior should be replicated:

if node properties were changed and the user selects another node, a prompt should be displayed, notifying the user

marthacryan · 2020-12-21T18:55:47Z

@ptitzler Thank you for the feedback!

We should mark required properties. If the widget doesn't support that (yet) natively, the info text should state if a value is required.

Good idea! Just added it. I don't think we marked any fields as required before this PR, does this look ok to you?

Probably there should not be empty default entries ^^ just like there is none for dependencies

I actually added that on purpose (I thought maybe it'd be easier to have that initial empty entry? The user isn't typing with the dependencies list so I removed this behavior for that one). But it's easy enough to remove, I'm not that attached to that behavior.

The button labels should be specific:Add item -> Add environment variable + ...

Good idea - just added that as well (see above)

ptitzler · 2020-12-21T19:41:50Z

I don't think we marked any fields as required before this PR, does this look ok to you?

In other places we specify (required), so for the sake of consistency the *q is probably not necessary.

Add Environmental Variable

Add Environment Variable

I actually added that on purpose

I see. I brought this up because of usability concerns:

We currently don't do this for other list-type inputs (like tags) anywhere else.
A user might think that they need to delete the "default" entry if they don't want to specify a value.
A user might think that they need to click Add XXX to "add" the entered value.
It takes up valuable screen real-estate, which will require more scrolling and clutter the interface, as we will be adding more properties. Even now, just test driving the implementation I find myself "maximizing" the side pane to make it feel less confining.

An unrelated question, it appears that we currently don't retain the size setting of the side bar:

add two nodes to the pipeline
open the properties of one node
maximize the size of the side panel
select another node -> the panel shrinks to its default size.
Imagine you are inspecting/browsing a pipeline with a dozen nodes. Having to re-size the side panel every time one switches to another node is getting rather quickly annoying. Is this something we have control over?

marthacryan · 2021-01-04T18:30:23Z

Inconsistency:

drag a notebook onto the canvas and define the required properties. save the changes. open the properties again.

select a different file in the properties view. save the changes. open the properties again. the file extension is gone

@ptitzler I found the code that causes this inconsistency - it appears to have been intentional to remove the extension, do you think it's better to remove the extension consistently or to keep the extension consistently? @ajbozarth @lresende I think I remember intentionally removing the file extension, did we stop doing that because of python file support in pipelines? (note: you can still tell which type of file it is based on the icon to the left of the label) I can change it either way just let me know.

ptitzler · 2021-01-04T18:43:32Z

I found the code that causes this inconsistency

🥳

it appears to have been intentional to remove the extension, do you think it's better to remove the extension consistently or to keep the extension consistently?

From a usability point of view I believe it'd be better to keep the file extension by default. If desired, a user can always change the node name to anything they'd like, such as an arbitrary string or just the file's base name.

marthacryan · 2021-01-05T17:26:37Z

Alternate idea for showing the longer filepaths:

karlaspuldaro

Really nice work! Just had a couple minor comments to add so far.

packages/pipeline-editor/src/PipelineEditorWidget.tsx

karlaspuldaro · 2021-01-05T22:50:57Z

packages/pipeline-editor/src/PipelineEditorWidget.tsx

@@ -1270,6 +1298,27 @@ export class PipelineEditor extends React.Component<
      this.handleOpenPipeline();
    });
  }
+
+  componentDidUpdate(): void {
+    const inputFields = document.querySelectorAll('.properties-readonly');


I wonder if there is a way to access how uihints are rendered from properties.json and use it instead of reading info from the DOM to create a tooltip from scratch?

I agree - I tried looking into it and this was my last option :/ And it seemed better than having a long text field

I might be missing something, but from investigating I found that you could have descriptions based on the value of fields: https://github.com/elyra-ai/canvas/wiki/3.2-Common-Properties--UI-Hints#dynamic-text-expressions, but I wasn't able to get it working and it seems like it might be only for creating a sum or percent?

I also looked into the readonly code and it seems like there's only a tooltip if the control is within a table? I don't know much about it and it doesn't seem to be documented anywhere.

We can always clarify that with the canvas team later, I was just curious if we could reuse their tool, good to know you've already tried something out :) I'm fine with keeping it as it for now.

marthacryan · 2021-01-06T20:21:40Z

@ptitzler The editor size being persisted is an issue on the canvas side but I opened an issue there: elyra-ai/canvas#436

marthacryan · 2021-01-11T23:22:54Z

@ptitzler You should be good to check it out again!

marthacryan · 2021-01-11T23:25:25Z

Updated to include auto-updating the properties instead of pressing an "OK" button.

Here's the tooltip that shows the full file path:

Here's the appearance in general:

ptitzler · 2021-01-12T00:02:14Z

Super. I'll test drive it tomorrow!

lresende · 2021-01-12T05:12:30Z

packages/pipeline-editor/src/properties.json

        }
      },
      {
        "parameter_ref": "dependencies",
        "label": {
          "default": "File Dependencies"
        },
-        "place_holder_text": {


In general, the long texts were to help self-document what users can expect/do with these fields. Are we removing/trimming them due to lack of space?

I moved this text into the description because I changed the UI and there is no longer placeholder text for dependencies. I kept the descriptions the same in general, but in this case, I removed two lines because "One filename or expression (e.g. *.py) per line. Supported patterns: ? and *." is unnecessary - now you add entries by pressing the "Add dependencies" button. (see below)

With this, are we not allowing the user to just type (.py)? What happens if the user tries to load a pipeline that already has (.py) on it? I believe we should still enable users to type (.py) or (.csv) as there might be a lot of files? What do others think?

I agree. It should still be possible to type in an expression, like it was possible in previous releases.

Oh ok - how do you think that should be structured? I can definitely make the fields editable. Should I add a separate button to create a blank field? I could add it next to the "Add dependencies" button, or it could be in the file browser dialog?

Something like this? (Add dependency creates a blank entry, Browse files opens the filebrowser)

This seems to one of the cases where a special kind of widget would work best - one that allows for the selection of a file or specification of an "arbitrary" string. In essence an amalgamation of what we use today select the input file and to type in output file specifications.

marthacryan · 2021-01-13T18:57:02Z

Updated UI for file dependencies.

lresende

LGTM

lresende · 2021-01-14T17:54:36Z

I will leave it open for the day in case others want to review it before I go ahead and merge.

karlaspuldaro

Looks great!
Maybe just check the warnings on the renderControl() function, otherwise LGTM

Replace dialog with side pane

f4650b5

Merge with master

08c5540

Add ability to change label

99ca9b3

Add description instead of placeholder

af010f3

lresende added the status:Work in Progress label Nov 23, 2020

Martha Cryan added 9 commits December 1, 2020 18:01

Merge branch 'master' of github.com:elyra-ai/elyra into add-side-pane

c20e9c8

Add some blueprint components

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.

Learn about vigilant mode

62326d1

Debugging scss

94848bb

add scss

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.

Learn about vigilant mode

8cafbf3

Update style

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.

Learn about vigilant mode

c51d635

Clean up code and update css

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.

Learn about vigilant mode

02ad1ce

Fix accept/cancel button style

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.

GPG key ID: 4AEE18F83AFDEB23
Expired

Verified
Learn about vigilant mode

5c8117a

Merge branch 'master' of github.com:elyra-ai/elyra into add-side-pane

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.

GPG key ID: 4AEE18F83AFDEB23
Expired

Verified
Learn about vigilant mode

38cea48

Update CSS for long paths

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.

GPG key ID: 4AEE18F83AFDEB23
Expired

Verified
Learn about vigilant mode

af70dbf

Cleanup package.json

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.

GPG key ID: 4AEE18F83AFDEB23
Expired

Verified
Learn about vigilant mode

c5476d5

Switch item for more specific label

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.

GPG key ID: 4AEE18F83AFDEB23
Expired

Verified
Learn about vigilant mode

2f666a3

Martha Cryan added 2 commits December 29, 2020 00:02

Merge branch 'master' of github.com:marthacryan/elyra into add-side-pane

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.

GPG key ID: 4AEE18F83AFDEB23
Expired

Verified
Learn about vigilant mode

fb4202e

Remove * from asterisk

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.

GPG key ID: 4AEE18F83AFDEB23
Expired

Verified
Learn about vigilant mode

5f24c6b

Change appearance of tooltip

6d29e30

karlaspuldaro reviewed Jan 5, 2021

View reviewed changes

Martha Cryan added 3 commits January 6, 2021 21:46

Fix tooltip arrow

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.

Learn about vigilant mode

f75761d

Merge branch 'master' of github.com:elyra-ai/elyra into add-side-pane

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.

GPG key ID: 4AEE18F83AFDEB23
Expired

Verified
Learn about vigilant mode

b0c1417

Add auto-update

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.

Learn about vigilant mode

b66fcb2

marthacryan marked this pull request as ready for review January 11, 2021 23:20

marthacryan requested a review from ajbozarth January 11, 2021 23:20

marthacryan changed the title ~~[WIP] Replace common properties dialog with side pane~~ Replace common properties dialog with side pane Jan 11, 2021

Martha Cryan added 2 commits January 11, 2021 18:03

Fix border issue and description

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.

Learn about vigilant mode

3001c17

Fix tests

9976bbf

lresende reviewed Jan 12, 2021

View reviewed changes

lresende removed the status:Work in Progress label Jan 12, 2021

Add browser button to each dependency

397af28

lresende approved these changes Jan 14, 2021

View reviewed changes

karlaspuldaro approved these changes Jan 14, 2021

View reviewed changes

lresende merged this pull request into elyra-ai:master Jan 15, 2021

lresende pushed a commit that referenced this pull request Jan 15, 2021

Replace pipeline node properties dialog with side pane (#1084)

6875efb

akchinSTC mentioned this pull request Feb 4, 2021

Provide better and more feedback to users when including local dependencies #568

Closed

Replace common properties dialog with side pane #1084

Replace common properties dialog with side pane #1084

Conversation

marthacryan commented Nov 18, 2020 • edited Loading

Siteproxy

搜索引擎

常用网站

新闻网站

海外论坛

elyra-bot bot commented Nov 18, 2020

kevin-bates commented Nov 18, 2020

marthacryan commented Nov 19, 2020 • edited Loading

marthacryan commented Dec 18, 2020 • edited Loading

ajbozarth commented Dec 19, 2020

ptitzler commented Dec 21, 2020 • edited Loading

ptitzler commented Dec 21, 2020

marthacryan commented Dec 21, 2020

ptitzler commented Dec 21, 2020 • edited Loading

marthacryan commented Jan 4, 2021 • edited Loading

ptitzler commented Jan 4, 2021

marthacryan commented Jan 5, 2021

karlaspuldaro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marthacryan commented Jan 6, 2021

marthacryan commented Jan 11, 2021

marthacryan commented Jan 11, 2021

ptitzler commented Jan 12, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marthacryan commented Jan 13, 2021

lresende left a comment

Choose a reason for hiding this comment

lresende commented Jan 14, 2021

karlaspuldaro left a comment

Choose a reason for hiding this comment

marthacryan commented Nov 18, 2020 •

edited

Loading

marthacryan commented Nov 19, 2020 •

edited

Loading

marthacryan commented Dec 18, 2020 •

edited

Loading

ptitzler commented Dec 21, 2020 •

edited

Loading

ptitzler commented Dec 21, 2020 •

edited

Loading

marthacryan commented Jan 4, 2021 •

edited

Loading