'Canvas View': pfCanvas & pfCanvasEditor - Contribution from MiQ/SUI #410

dtaylor113 · 2017-02-12T17:35:02Z

Ok, this is HUGE! -as in a lot of code 😄 Six months work plus the original open source core lib code.

pfCanvasEditor

In Connection Mode:

Some Notes:

As stated in the ngDocs, this does NOT work in IE 11 or lower due to IE's lack of support for latest SVG specification. Does work in FireFox, Chrome, and MS-Edge.
pfCanvasEditor is a Angular 1.5 Component as it is the main/top component to use. pfCanvas and other sub components are Angular 1.x Directives.
There are several unit tests in MiQ/SUI, but they need to be modified to be used for pfCanvas & pfCanvasEditor. I will create a JIRA story for the unit tests, as well as several other stories for follow up work for pfCanvas & pfCanvasEditor:
PTNFLY-2193: Unit Tests
PTNFLY-2197: Remaining Issues (contains links to the other Canvas View JIRA stories)

serenamarie125 · 2017-02-13T03:16:06Z

@dtaylor113 thanks for finishing this off so quickly! AFAIK, the remaining issues in PTNFLY-2197 are not needed. @catrobson, can you confirm?

dtaylor113 · 2017-02-13T14:14:34Z

@serenamarie125, @catrobson, ok on PTNFLY-2197, but just want to make sure you see the other 'Canvas View' stories which are linked to PTNFLY-2197:
PTNFLY-2194: Canvas View: Toolbox 'leaveCopy' functionality (AngularJS)
PTNFLY-2195: Canvas View: Panning functionality/cursor (AngularJS)
PTNFLY-2196: Canvas View: Left Toolbox (AngularJS)
Thanks,

Dave

serenamarie125 · 2017-02-17T12:15:50Z

@bleathem @jeff-phillips-18 @dlabrecq @dgutride is it possible to get some eyes on this? This is something that we pushed to get @dtaylor113 to complete last week, since once of Cat's products is waiting for it.

jeff-phillips-18

Lots of code! I went through it best I could given the size and time constraints.

There are directives being added which should be components. This version of angular patternfly should have no directives (or at least only those that are not components).

Concerned about including JQuery but since the module is optional, I guess it's OK as long as we don't pull any jquery packages by default.

jeff-phillips-18 · 2017-02-17T12:57:37Z

package.json

@@ -12,6 +12,9 @@
    "angular-animate": "1.5.*",
    "angular-sanitize": "1.5.*",
    "angular-ui-bootstrap": "2.3.x",
+    "angular-dragdrop": "1.0.13",
+    "angular-svg-base-fix": "2.0.0",
+    "angular-dragdrop": "1.0.13",


Included angular-dragdrop twice. Should this be a dependency or devDependency? If it is Opt In, maybe better to be a devDependency so applications not using the canvas do not get this, those that opt in will have to specifically include it.

jeff-phillips-18 · 2017-02-17T13:03:08Z

src/canvas-view/canvas-editor/canvas-editor.component.js

+        newNodeCount++;
+        newNode.x = event.clientX - 600;
+        newNode.y = event.clientY - 200;
+        newNode.backgroundColor = newNode.backgroundColor ? newNode.backgroundColor : '#fff';


This should use pfUtils.colorPalette.white

Weird, but there isn't a 'white' in patternfly.pfPaletteColors

Well that is silly...

jeff-phillips-18 · 2017-02-17T13:03:21Z

src/canvas-view/canvas-editor/canvas-editor.component.js

+        newNodeCount++;
+        newNode.x = 250 + (newNodeCount * 4 + 160);
+        newNode.y = 200 + (newNodeCount * 4 + 160);
+        newNode.backgroundColor = newNode.backgroundColor ? newNode.backgroundColor : '#fff';


This should use pfUtils.colorPalette.white

jeff-phillips-18 · 2017-02-17T13:24:55Z

src/canvas-view/canvas-editor/canvas-editor.less

+        }
+      }
+      .not-draggable {
+        background-color: #ebebeb;


Palette color?

jeff-phillips-18 · 2017-02-17T13:27:47Z

src/canvas-view/canvas/canvas-directive.js

+  // Directive that generates the rendered chart from the data model.
+  //
+  .directive('pfCanvas', function ($document) {
+    return {


This should be a component.

I agree, however it is not a requirement for this initial PR which requires pfCavnasView to be a component to be accessed in an Angular2 'hybrid mode'. Converting pfCanvas to a component would allow Angular2 folks to access the basic canvas component (without toolbox & zoom) -which I'm not sure is a use case. I've captured this in PTNFLY-2197: Remaining Issues

I believe the requirement for the 4.x release was to use components. We converted all the directives to components and don't think we should, at this point, introduce new directives.

Again, I agree, but it will take a significant rewrite of that core directive in order to convert it to a component. The main pfCanvasEditor is a component and is what end users can/will access from a hybrid angular 2 setup. @dgutride and I talked about that child/sub directives used in component templates were ok (not ideal), as long as the parent was a component.

OK, I would have preferred not to introduce directives but I guess this could be refactored at a later time.

jeff-phillips-18 · 2017-02-17T13:34:50Z

src/canvas-view/canvas/canvas.less

+.invalid-node-header {
+  fill: #b1b3b6;
+  color: #b1b3b6;
+}


Palette colors

jeff-phillips-18 · 2017-02-17T13:35:18Z

src/canvas-view/canvas/canvas.less

+  fill: transparent;
+  stroke-width: 2;
+  stroke: #0000ff;
+}


Palette color?

jeff-phillips-18 · 2017-02-17T13:35:42Z

src/canvas-view/canvas/canvas.less

+  position: relative;
+  &::after {
+    border-bottom-color: @color-pf-white;
+    border-color: #333333;


Palette color

jeff-phillips-18 · 2017-02-17T13:37:19Z

src/canvas-view/examples/canvas.js

+         width: 99%;
+     }
+   </style>
+   <div ng-controller="CanvasDemoCtrl" class="example-container">


These styles should go into the examples css

jeff-phillips-18 · 2017-02-17T13:38:46Z

src/canvas-view/examples/canvas.js

+              "y": 67,
+              "id": 1,
+              "image": "/img/OpenShift-logo.svg",
+              "width": 150,


We should avoid using project related images and use generic images and names.

These are included in core Patternfly dist/img.

Understood. My suggestion was to use generic items rather than storage specific items ie 'Dolor' rather than 'OpenShift'

jeff-phillips-18 · 2017-02-17T13:56:22Z

Some alignment issues here:

Hide connectors is not centered on the toggle
Filter by name box should have some left padding before the text
Tabbed content should have some left padding from the tab area
The +/- buttons should have hover treatments
The duplicate and delete buttons should have hover treatments (when enabled)

jeff-phillips-18 · 2017-02-17T14:04:11Z

Besides clicking on the Add Item again to close the add area, we should put a dismiss button in the upper right (x).

Clicking on anywhere on the Canvas will close the Toolbox as well.

jeff-phillips-18 · 2017-02-17T14:05:07Z

It is odd to me to have the filter below the items, everywhere else we have filtering above.

dtaylor113 · 2017-02-17T15:53:26Z

Hi, addressed most issues raised by Jeff. @serenamarie125 , please let me know your thoughts on Jeff's last two comments on the 'X' to close the toolbox and the placement of the toolbox filter. Please let me know if you'd like me to work on these now, or add them to the JIRA story. -thanks

jeff-phillips-18 · 2017-02-17T18:44:38Z

src/canvas-view/canvas-editor/canvas-editor.less

@@ -39,7 +39,7 @@
      vertical-align: middle;
    }
    .show-hide-connectors-label {
-      vertical-align: text-bottom;
+      vertical-align: 2px;


Removing the vertical-align setting all together seems to make it line up better.

Not for me? Seems to bottom align.

If I remove the setting I get:

jeff-phillips-18 · 2017-02-17T18:54:48Z

src/canvas-view/canvas-editor/canvas-editor.less

+    position: relative;
+    width: 100%;
+    .canvas-editor-toolbox {
+      background-color: rgba(255, 255, 255, 0.94);


Do we want less than full opacity for this? It looks odd to me having the node items bleed through.

Hmm..the original mockups specifically showed the items showing through the toolbox...

I'm looking at https://github.com/patternfly/patternfly-design/blob/master/pattern-library/content-views/canvas-view/design/design.md

I see the x to dismiss and a gray background not translucent.

No, I'm talking way back to Colleen's original InVision mockups. I'm not sure the current design you mention above actually shows nodes behind the toolbox, and that they are hidden by the toolbox.

Either way is fine with me, easy enough to change the value 😄

That design is rather recent, and is somewhat incomplete as it doesn't show the Filter in the Toolbox or the Hide Connections checkbox.

bleathem · 2017-02-17T19:01:17Z

Thanks for the thorough review @jeff-phillips-18!

serenamarie125 · 2017-02-17T19:10:23Z

@dtaylor113 those two remaining issues @jeff-phillips-18 requested sound to be low hanging fruit. Are they things that could be knocked off quickly ?

dtaylor113 · 2017-02-17T19:50:28Z

@serenamarie125, it was more a question of if you agree with the changes. I can start work on them now. Just don't know if this is holding up a release? ( @bleathem )

dtaylor113 · 2017-02-17T20:26:28Z

Done. New placements:

serenamarie125 · 2017-02-17T20:47:01Z

I meant the "x" to close the add and the vertical alignment of the hide connectors :) @dtaylor113

serenamarie125 · 2017-02-17T20:47:42Z

I think the other things we can hold off. Media Drive should be able to use these, and we can do a secondary review on this later on, imo.

serenamarie125

LGTM! I'm going to note that we need to schedule a design review of this after this merge, and if additional issues are found, we will create an Issue or add to pivotal. Thanks @dtaylor113 !

jeff-phillips-18

👍

jeff-phillips-18 requested changes Feb 17, 2017

View reviewed changes

jeff-phillips-18 reviewed Feb 17, 2017

View reviewed changes

dtaylor113 added 2 commits February 17, 2017 15:24

pfCanvas & pfCanvasEditor: Contribution from MiQ/SUI

6d46ad1

Moved filter to top of toolbox, added 'X' close.

982290f

dtaylor113 force-pushed the CFUX-308-pfCanvas branch from ced8a7a to 982290f Compare February 17, 2017 20:25

serenamarie125 approved these changes Feb 17, 2017

View reviewed changes

jeff-phillips-18 approved these changes Feb 17, 2017

View reviewed changes

jeff-phillips-18 merged commit 14bd794 into patternfly:branch-4.0-dev Feb 17, 2017

dtaylor113 deleted the CFUX-308-pfCanvas branch April 20, 2017 14:28

'Canvas View': pfCanvas & pfCanvasEditor - Contribution from MiQ/SUI #410

'Canvas View': pfCanvas & pfCanvasEditor - Contribution from MiQ/SUI #410

Uh oh!

Conversation

dtaylor113 commented Feb 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

serenamarie125 commented Feb 13, 2017

Uh oh!

dtaylor113 commented Feb 13, 2017

Uh oh!

serenamarie125 commented Feb 17, 2017

Uh oh!

jeff-phillips-18 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jeff-phillips-18 commented Feb 17, 2017

Uh oh!

jeff-phillips-18 commented Feb 17, 2017 • edited by dtaylor113 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jeff-phillips-18 commented Feb 17, 2017

Uh oh!

dtaylor113 commented Feb 17, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dtaylor113 Feb 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bleathem commented Feb 17, 2017

dtaylor113 commented Feb 12, 2017 •

edited

Loading

jeff-phillips-18 commented Feb 17, 2017 •

edited by dtaylor113

Loading

dtaylor113 Feb 17, 2017 •

edited

Loading