elyra-ai / pipeline-editor Goto Github PK

Since the pipeline button is part of the svg, if you accidentally move your cursor will clicking it just drags the canvas. We should use a standard button instead.

Currently, the language server highlights the entire object when missing a property value. We should only highlight the key so it's less distracting.

Do this:

"key": { 
   "key": "value",
   "key": "value",
   "key": "value"
}

instead of this:

"key": { 
   "key": "value",
   "key": "value",
   "key": "value"
}

I haven't done much dogfooding of supernodes yet and have a feeling we might have some bugs with validation

clicking on another node after typing into a node property field doesn't save the data. You have to click somewhere else first to trigger the save

When the filename is not undefined, but is an empty string, it uses the filename instead of falling back to the default label

All commands should either be in make or npm scripts

Waiting on a decision from elyra-ai/elyra#1532

We should do some preprocessing before we serialize property values.

Examples

Number:

• "123" -> 123
• "1.23e2" -> 123
• "123.00" -> 123
• "" -> undefined
• " " -> undefined
• "one" -> undefined

String:

• "" -> undefined
• " " -> undefined
• " hello " -> " hello "

Issue

If we clean data before storing, we run into problems. This is because there is a 1-to-1 binding between data and UI. A change to the data changes the ui and vice-versa. Take number serialization as an example:

user types: "1", 1 is stored, ui updates to render "1"
user types: "12", 12 is stored, ui updates to render "12"
user types: "12.", 12 is stored, ui updates to render "12"

oh no we can't type decimals because string -> number is a lossy conversion!

Solution

We will need to introduce a new data binding model that uses a local state to track what the user has actually typed so far, so we don't blow away whatever they are inputting every key press.

Only use local state when input control is focused, when blurred clear local state and show actual data

Styling pipeline-editor with pure css can cause webpack issues.

Additionally it should make theming simpler

This would also be a good opportunity to add a better way to to use svg icons for various icon buttons

Currently, string array properties in a pipeline file are just a plain array of strings. However, we need stable IDs for our string/file array form control to safely render/edit values. This is a breaking change, so we need to increment the current pipeline version from 3 to 4.

this causes a bug where a node isn't created when trying to add a file to the canvas via right click.

@lresende I think this is the bug we were seeing in vscode

Add basic installation and contribution information to README

current context menus:

background:

New comment
Select All
-----------------
Edit > Cut
       Copy
       Paste
-----------------
# Undo
# Redo
-----------------
# Unhighlight

Node:

Disconnect
-----------------
Edit > Cut
       Copy
-----------------
Delete
-----------------
Create supernode
-----------------
Highlight > Highlight Branch
            Highlight Upstream
            Highlight Downstream
            -----------------
            # Unhighlight
Open File
Properties

Link:

Delete

Comment:

Disconnect
-----------------
Edit > Cut
       Copy
-----------------
Delete
-----------------
Create supernode

Small issues:

• There is a mix of casing style One two three vs One Two Three
• There seems to be no logic of when an item is completely hidden vs disabled
• There doesn't seem to be a lot of logic behind the dividers, some things are grouped that don't make sense (highlight, open file, properties) where most of the time there is a divider between everything.
• Context menus don't disappear on blur

Questions:

• Is the highlight menu useful? I didn't realize what it did until today. I always thought it had something to do with git so I just ignored it. Additionally, I would assume highlighting something would act as selection, but highlighting a branch and then deleting only deletes the selected node, not the highlight. I motion to delete this menu in vscode release to see if it's something people actually want, because to me, it just adds confusion.
• Do we need the three dot button on the nodes? It seems to do the exact same thing as right click? Is there an accessibility reason? If so why don't we have the same on links, comments, etc...

Suggestions:

• Remove Undo, Redo, Select All. These types of actions normally live in an edit menu or toolbar
• Remove the overflow for Edit, this should just be a group of Cut, Copy, Paste
• Remove most of the dividers, the only divider should probably be the edit group. I think a good rule of thumb should be if you can't think of a generic name for the group it shouldn't have a divider (in most cases there shouldn't be groups of one item, because then the divider just creates noise, but there can be exceptions to this rule)

Jupyter vs VSCode

Jupyter:

• uses their own context menu ui
• uses icons to the left for a lot of menu items
• has keyboard shortcut to the right
• never uses dividers
• rarely uses an overflow menu

VSCode:

• uses system context menu, but doesn't expose the API to custom web editor views
• uses dividers sparingly
• rarely uses an overflow menu

currently the file extension option for a file is a single file extension, but this should be an array

blocked by open PRs

Currently we only have test coverage for the pipeline-services package

Currently number fields can be added to the parameter definition of nodes, but the style is not quite right and the value doesn't save when edited. Maybe there should be a custom form control?

currently the list of nodes in the palette is just text. Add the node's icon to the left of the text

Notebooks created in vscode don't have a kernel spec until you run them, which causes issues when trying to run them with elyra

VS Code example:

Note: VS Code does not persist invalid values. For example, if the valid was 14 and you typed 130 the persisted value would be 13. Currently, this won't work with the fully controlled/data driven nature of our forms. If 130 isn't persisted we wouldn't be able to type 130. We would have to create a custom component with an error state that tracks the invalid value. Something like:

const [value, setValue] = useSelector() // 13
const [invalidValue, setInvalidValue] = useState() // 130

...

<input value={invalidValue ?? value} />

We only have a placeholder cypress test at the moment. Write some more thorough integration tests

For two main reasons:

1. When an out-of-date pipeline is opened a notification is shown, asking if they want to migrate. The migration will only happen if the pipeline is the active editor. We should probably move the migration code to the backend so the migration action will happen regardless.
2. For vscode we will need to build a language server to surface any pipeline issues.

make list items clickable and when clicked, show options (edit/browse/delete)

onError should be a callback with a signature of (title, body) => any so we have consistent copy across platforms

Make sure everything is styled by a classname so that the ui can be properly themed for their environments

• pipeline
• right panel
  • tabs
  • common properties
  • palette

Currently, simple string inputs aren't being trimmed, for example typing "     " is considered non-empty. We should use value.trim() === "" instead of value === "", This is more consistent with our string array inputs

VSCode has it's own built-in undo/redo so we should disabled the canvas built-in ctrl+z

The handleClickAction handler can't be defined by a parent of PipelineEditor

• We should validate that the file exists, how often? Do we need to watch the file somehow?
• Should we change the file label to just be the basename? xxx.ipynb feels a bit noisy...
• If a file is renamed should the node's file path also be updated?

• update the style
• grouping
• searching
• filtering

Make sure a user can create a blank file with a .pipeline extension and be able to edit the pipeline file

Blocked until we merge some of the open PRs

This might be challenging to do. In order to enable this feature we would need to:

1. Delegate verification of the file path to the caller of pipeline-service/validate which would need to be bubbled higher in the case of usage in pipeline-editor
2. Set up file watchers to retrigger validation when file paths change

• The text is still black in dark themes
• The open pipeline button is unthemed
• return to previous flow button is unthemed

As an existing JupyterLab user:

• One would expect to be able to drag a file onto the canvas from the left file panel (not possible in VS Code)
• They might already have a pipeline file from JupyterLab so an empty canvas message won't help here
• Since they are a current user, they might not be following along with any documentation/tutorials

As a new user:

• Have no habits from other elyra platforms
• More likely to have an new pipeline so they would see a help text on the empty canvas
• Might be following along with documentation/tutorial with instructions on how to add nodes

We have a few established user experiences that are either too "opinionated" or too established to modify. We should introduce settings to test out alternatives. Some examples:

• file extensions: This is 100% a user preference. The user should be able to decide if they want to show the full file name example.ipynb or the name without the extension example. (default: show file extension)
• double click behavior: Double clicking on a file opens a node, but this is arguably not the best for various reasons (not all nodes have a file tied to them, clicking on a node changes the active properties, opening a flyout panel is an established pattern in other products, etc...). However, current Elyra users may be too accustomed to it opening a file. The user should be able to decide whether double click opens properties or opens the file. (default: open properties)
• scroll to zoom: The user should be able to decide whether or not the canvas will zoom in/out when scrolling, this behavior might not be used by many and the effect of an accidental scroll can be a bit jarring for some users. (default: disabled)

"enter/return" should trigger "OK"
"esc" should trigger "Cancel"