Add indicator for invalid node properties

[marthacryan]

Fixes #653. Adds a small red caret to nodes with invalid properties. Only checks
for having a runtime image chosen for now. Looks like this:

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.

[lresende]

One thing here is the question of using warning vs something more like error as when the full validation is added this should/would disable pipeline submission if some of these are present.

As for the icon, how about something more like a red circle or red x or red * on the top right corner aligned a little farther from the border?

Note that the icon is more like a suggestion.

[vabarbosa]

updated the icon used and its position.

the icon can be replaced if desired by replacing the packages/ui-components/style/icons/error.svg.
alternative icons can be found here: https://www.carbondesignsystem.com/guidelines/icons/library/
(just search for error or alert)

[ajbozarth]

The update location of the icon is better, but looking at the carbon icons I think a "filled" icon would be slightly easier to see at that size, like "error filled" or "warning filled"

I'll do a more detailed review soon

[ajbozarth]

I ran this locally and feature-wise it works great, but I do have a couple of UX comments.

In addition to my previous comment about using a "filled" icon I also found myself trying to click on the error icon in an attempt to figure out what's wrong. Currently this doesn't do anything, perhaps we should try to tie some functionality into clicking the icon, maybe have it open the tool tip which displays the error? Currently you have to mouse over the node for a couple second for the tooltip to show up, but clicking the node (including the icon) will cause the tooltip to not show until you move the mouse off and onto the node again.

[lresende]

@ajbozarth Hovering on the node should show you the properties and what is wrong there.

[ajbozarth]

@lresende that is correct, but if a user clicks on it before the tooltip appears it selects the node instead and the tooltip will not appear at that point until a user mouses off and on the node again

[marthacryan]

Options for icon:



Thoughts? @vabarbosa @ajbozarth

[ajbozarth]

Of those three options I would choose the first one (red dot). Is a "filled" version of the second one not an option though?

(Edit: I'm thinking specifically if the Error Filled carbon icon)

[vabarbosa]

since the icon is small i would vote to keep it simple and go with the red dot.

[vabarbosa]

for comparison purposes this is how it is done in canvas (Test harness)

• Text/label color changed
• Error Filled icon near the node's icon

[marthacryan]

@vabarbosa @ajbozarth I've been playing around with the different options this morning - thoughts?

[vabarbosa]

@vabarbosa @ajbozarth I've been playing around with the different options this morning - thoughts?

very nice!

i like this with a red border:

or this as is:

[marthacryan]

This PR now depends on #744 for the error alert at the top of the pipeline.
Here's the updated behavior / look:
When you drag nodes onto the canvas, they look normal by default:

After pressing the run button, they look like this:

(Wasn't sure how to phrase that error message at the top - it says "Invalid pipeline: some nodes have incomplete properties")

After fixing the properties for one of the nodes, it removes the error indicator for just that node:

After updating any of the properties of the nodes, it shows indicators for all nodes without the needed properties.

[marthacryan]

Discussion:
Should we be performing the same checks for exporting pipelines?

[kevin-bates]

The screenshots and error message look great!

Yes, I believe this should apply to export as well. Anything that saves the user time is helpful.

[vabarbosa]

looks great!

a couple points. perhaps the message should read

Invalid pipeline: Some nodes have missing or invalid properties.

or something along those lines (other may have better suggestions for the phrasing).

i would agree and say similar checks should be done on exporting. however (unless it will cause errors in the backend), i don't believe it should prevent the user from exporting. just warn them so they are aware before they continue.

[marthacryan]

Last commit changes name of validation functions to validateNode and validateAllNodes. @lresende This should be good to merge as soon as @ajbozarth adds his approval and #744 is merged (already rebased off of those changes).

[vabarbosa]

just made couple comments

[vabarbosa]

Looks good. thanks.

[marthacryan]

New feature based on case brought up in #744 - checks for circular references on startup and adds an indicator for all links in a cycle, as well as an error message.

Code changes

Adds a function validateAllLinks and a function validatePipeline. validatePipeline returns a tidy error message that collects the error messages from both validateAllNodes and validateAllLinks, and returns null if everything is valid.

Behavior changes

Here's what it looks like when there are both invalid links and invalid nodes and you try to run, export, or just open the pipeline:

After fixing the cycle, it automatically removes the style from other links:

[vabarbosa]

@marthacryan this looks great! thank you for the quick turn around.