fix(Canvas): Canvas directive to component implementation #640

amarie401 · 2017-10-05T18:00:16Z

Description

Conversion of the Canvas directive to a component. This fixes #636

dtaylor113

Hi Amiee, this is great! One thing I noticed is that the keys don't seem to be working Ctrl+A to select all, [Delete] key to delete currently selected, etc.
I noticed in canvas-component.js you moved the keystroke handling to an $onInit, maybe try in $postLink?

dtaylor113 · 2017-10-05T19:05:37Z

Other than the keystroke issue, LGTM 👍 !

amarie401 · 2017-10-05T19:06:30Z

Thanks @dtaylor113 going to look into that issue now

jeff-phillips-18 · 2017-10-05T20:01:11Z

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

+          var ctrlKeyCode = 17;
+          var ctrlDown = false;
+          var aKeyCode = 65;
+          var dKeyCode = 68;


this is not used

jeff-phillips-18 · 2017-10-05T20:01:54Z

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

+        //
+        // zoom.
+        //
+        $scope.$on('zoomIn', function (evt, args) {


arguments are not used

jeff-phillips-18 · 2017-10-05T20:02:05Z

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

+          ctrl.chart.zoom.in();
+        });
+
+        $scope.$on('zoomOut', function (evt, args) {


arguments are not used

amarie401 · 2017-10-05T20:37:33Z

@dtaylor113 keys should be working now. It was using scope instead of $scope and there was an issue where the mac keyboards use the backspace keycode for "delete" so had to account for both. It wasn't working for me for that reason.

dtaylor113

LGTM! Thanks

cdcabrera

There looks like some broken functionality around the "events" demo display... where there should appear to be something like (in both the canvas and the editor version)...

cdcabrera · 2017-10-09T14:27:53Z

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

+
+        ctrl.clickCallbackfmDir = function (item) {
+          if (!item.disableInToolbox) {
+            ctrl.clickCallback(item);


This may have been in the existing directive, but if we're going for consistency...

we do checks for a function within the node-toolbar-component.js should we be doing that here too?
Similar too...

if (angular.isFunction(ctrl.clickCallback)) {...

Can do. Makes sense for consistency purposes.

After looking into it, seems like ctrl.clickCallback isn't a function so would be unable to check for a function in this instance

If it isn't a function, how is it being called?

Or maybe I'm misunderstanding

cdcabrera · 2017-10-09T14:28:44Z

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

+        };
+
+        ctrl.startDragCallbackfmDir = function (event, ui, item) {
+          ctrl.startDragCallback(event, ui, item);


Same call out on function check...

if (angular.isFunction(ctrl.startDragCallback)) {...

cdcabrera · 2017-10-09T14:30:44Z

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

+        startDragCallback: '=',
+        clickCallback: '=',
+        searchText: '='
+      },


One-way binds for the callbacks using < do they work? Or do we need some form of the function/callback passed back out?

The reasoning typically has to do with "watches" and "parents" then add on top of that a callback which kind of works around a watch...

The one-way binds seem to work in this case.

@jeff-phillips-18 @dtaylor113 since the syntax on the consumption end should be the same is there a contrast opinion to updating the syntax?

works for me.

cdcabrera · 2017-10-09T14:32:45Z

src/canvas-view/canvas/node-toolbar.component.js

+        nodeActions: '=',
+        nodeClickHandler: '=',
+        nodeCloseHandler: '='
+      },


Same comment regarding using one-way binds instead of two-way on the -Handlers using <, do we need the handlers passed back out?

From my knowledge, I don't believe the handlers need to be passed back out. Can change these to one-way binds as well.

jeff-phillips-18 · 2017-10-09T15:04:27Z

src/canvas-view/canvas/node-toolbar.component.js

+        node: '=',
+        nodeActions: '=',
+        nodeClickHandler: '=',
+        nodeCloseHandler: '='


We should add comments here (since there is no doc for this) as to what the callback arguments should be.

jeff-phillips-18 · 2017-10-09T15:04:34Z

src/canvas-view/canvas/node-toolbar.component.js

+        ctrl.actionIconClicked = function (action) {
+          ctrl.selectedAction = action;
+          if (angular.isFunction(ctrl.nodeClickHandler)) {
+            ctrl.nodeClickHandler({'action': action, 'node': ctrl.node});


Might be better to pass 2 arguments rather than an object.

jeff-phillips-18 · 2017-10-09T15:12:34Z

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

+
+          $document.find('body').keydown(function (evt) {
+            if (evt.keyCode === ctrlKeyCode) {
+              ctrlDown = true;


You can check evt.ctrlKey to determine if ctrl is pressed rather than keeping state.

jeff-phillips-18 · 2017-10-09T15:13:30Z

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

+              evt.stopPropagation();
+              evt.preventDefault();
+            }
+          });


Why keyup sometimes and keydown others? I would think we could check only on keydown for all of these.

amarie401 · 2017-10-09T19:22:47Z

@cdcabrera @jeff-phillips-18 made some requested changes. Still working on fixing the eventText issue.

jeff-phillips-18 · 2017-10-09T19:24:00Z

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

+        ctrl.clickCallbackfmDir = function (item) {
+          if (angular.isFunction(ctrl.clickCallback)) {
+            if (!item.disableInToolbox) {
+              ctrl.clickCallback(item);


I would && these together for readability.

jeff-phillips-18 · 2017-10-09T19:25:06Z

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

+      controller: function toolboxItemsController () {
+        var ctrl = this;
+
+        ctrl.clickCallbackfmDir = function (item) {


This is an odd name for the function, what does the 'fm' mean?

I have no idea. They were already named these lol. Will change them.

That stood for 'from directive'. I think at some point just 'clickCallback' wasn't descriptive enough.

cdcabrera

Minor updates, an opinion question or two, and some props!

cdcabrera · 2017-10-10T14:06:44Z

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

+        chartViewModel: "=?",
+        readOnly: "=?",
+        hideConnectors: "=?"
+      },


We seem to flip back and forth between double quotes and single without a clear reason? Do you have a preference, or you up for making them consistent? If so encourage you to go back through and check them.

I'm fine with ~~double quotes~~. I can go back and make them consistent.
Note: single quotes :)

cdcabrera · 2017-10-10T14:12:02Z

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

+              evt.stopPropagation();
+              evt.preventDefault();
+            }
+          });


Just a note you don't need to change it.

This could have also been flipped to a switch. However it probably wouldn't save us much...

switch(evt.keyCode) { case deleteKeyCode: case backspaceKeyCode: ctrl.deleteSelected(); $scope.$digest(); break; case escKeyCode: ctrl.deselectAll(); $scope.$digest(); break; case ctrlKeyCode: evt.ctrlKey = false; evt.stopPropagation(); evt.preventDefault(); break; }

I'm fine with changing it to a switch..butttt looks the eslint is kinda funky when trying to implement it.

cdcabrera · 2017-10-10T14:18:56Z

src/canvas-view/canvas/node-toolbar.component.js

+      bindings: {
+        node: '=',
+        nodeActions: '=',
+        nodeClickHandler: '<', // @callback args - (action {string}, node {Object})


We sure about calling them arguments... what about parameters? (Looking for your input)

Also it may be a good idea to be consistent and apply similar comments to other callbacks that pass arguments (through parameters) back.

I thought args/params were pretty much the same thing? I actually documented the nodeClickHandler in ng-docs canvas.js example so might not need this comment next to it.

reworded the ng-docs to encompass the concept of both arguments and parameters for the nodeClickHandler in the canvas.js example. Do we still need the comment?

cdcabrera · 2017-10-10T14:19:42Z

src/canvas-view/examples/canvas.js

@@ -163,7 +169,7 @@
                    "fontFamily": "PatternFlyIcons-webfont",
                    "fontContent": "\ue621"
                  }
-                ],
+                ]


Good catch on going through the examples and cleaning up! Thank you!

cdcabrera · 2017-10-10T14:21:36Z

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

+          return point.matrixTransform(matrix.inverse());
+        };
+
+        ctrl.hideConnectors = ctrl.hideConnectors ? ctrl.hideConnectors : false;


You'll have to check it to confirm, but this might be a good spot for some simplification...

ctrl.hideConnectors = ctrl.hideConnectors || false;

If the idea is that any value makes this true, then I would use:

ctrl.hideConnectors = !!ctrl.hideConnectors;

It appears that it’s already considered a boolean, I could cast it but I think the syntax is a bit confusing

jeff-phillips-18 · 2017-10-10T15:05:16Z

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

+          var escKeyCode = 27;
+
+          $document.find('body').keydown(function (evt) {
+            if (evt.ctrlKey === true) {


(this got lost by an update)

You can check evt.ctrlKey to determine if ctrl is pressed rather than keeping state.

jeff-phillips-18 · 2017-10-10T16:28:20Z

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

+        ctrl.$onInit = function () {
+          var backspaceKeyCode = 8;
+          var deleteKeyCode = 46;
+          var ctrlKeyCode = 17;


This is not needed.

dtaylor113 · 2017-10-10T17:24:24Z

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

+              $scope.$digest();
+            }
+
+            if (evt.keyCode === ctrlKeyCode) {


You should replace this with evt.ctrlKey === true like in the Ctrl-A usage. Then you can get rid of the ctrlkeyCode variable like @jeff-phillips-18 suggests. -thanks!

Will do! Thanks :)

dtaylor113 · 2017-10-10T17:28:25Z

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

+              evt.stopPropagation();
+              evt.preventDefault();
+            }
+


You can get rid of the if (evt.ctrlKey === true)...stopPropagation statement since you are no longer keeping track of ctrl key state.

cdcabrera

Looking really good!

Keep seeing a broken image around the "dotted" background from /img/canvas-dot-grid.png from when looking at Rawgit, though it appears consistent with the current version, so working as intended. If you can figure it out go for it

cdcabrera · 2017-10-11T13:29:32Z

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

+      bindings: {
+        chartDataModel:'=',
+        chartViewModel: '=?',
+        readOnly: '=?',


This is non-breaking, but we use a one way bind on the other readOnly binding would that work here too?

Yes it does work here, will change for consistency.

dtaylor113 · 2017-10-11T13:44:34Z

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

+
+          $document.find('body').keydown(function (evt) {
+
+            if (evt.keyCode === aKeyCode) {


Hi, you need evt.ctrlKey === true here in order to do Ctrl-A, at the moment just hitting 'a' alone will select all.

ahh yes good catch!

amarie401 · 2017-10-11T14:24:36Z

We don't seem to unit test this set of components, is that something we could aim for?

@cdcabrera @jeff-phillips-18 @dtaylor113

cdcabrera · 2017-10-11T14:43:12Z

Unit tests eh, well they might have been helpful with the conversion as a base line comparison. But since they aren't there we could two part it @dtaylor113 @jeff-phillips-18 , were there any limitations before?

I'm leaning more towards integrating them into this PR, at least a few basics to verify things are loading/showing up. It'll increase the length of time on the PR a bit, but it'll head towards the sweet spot of a well rounded/thought out piece.

jeff-phillips-18 · 2017-10-11T14:48:51Z

@amarie401 I'm OK with adding unit tests as part of this PR or as a follow on.

cdcabrera

Follow up PR for testing sounds like the vote. That works, let's do it!

amarie401 · 2017-10-11T14:54:20Z

Sounds good! Thanks guys!

jeff-phillips-18 · 2017-10-11T14:56:47Z

One question for @amarie401 @dtaylor113 @cdcabrera , is this a fix or a chore ?

dtaylor113 · 2017-10-11T15:02:46Z

One question for @amarie401 @dtaylor113 @cdcabrera , is this a fix or a chore ?

I would think chore as underlying changes should be transparent to the end users.

amarie401 · 2017-10-11T15:03:13Z

Hmmm...I would say that it's a chore since it's a conversion and all the major pieces are still working.

jeff-phillips-18 · 2017-10-11T15:04:27Z

OK @amarie401, could you reword your commit please?

cdcabrera · 2017-10-11T15:04:56Z

heh, it kinda fits into neither. The conversion, while it shouldn't have, still might have introduced something that breaks... I'm more partial to labeling it a fix since there's at least a version increment for it that can be rolled back

Edit: Labeling this chore if we can work the unit tests in now feels better... but yeah my vote is for fix since it'll increment and allow consumers to pick their version

dtaylor113 · 2017-10-11T15:07:58Z

Wow, really thought I had added the unit tests, as there are extensive ones here and here
But I guess these existing 'blueprint canvas' unit tests would need to be updated to work with the generalized a-pf pfCanvas component. Created Issue #643 which can be addressed in a follow on PR.
-thanks

dtaylor113 · 2017-10-11T15:12:08Z

I'm ok with fix(..) as it may possibly introduce a regression.

amarie401 · 2017-10-11T15:13:32Z

In that case with the possibility of the rollback then I would agree on labeling it a fix, thoughts @jeff-phillips-18 ?

jeff-phillips-18 · 2017-10-11T15:26:29Z

OK, I can go along with that.

amarie401 force-pushed the canvas-to-component branch from c38452f to 04fcb59 Compare October 5, 2017 18:18

dtaylor113 requested changes Oct 5, 2017

View reviewed changes

amarie401 force-pushed the canvas-to-component branch 2 times, most recently from 5fa5db1 to 98112ab Compare October 5, 2017 19:57

jeff-phillips-18 reviewed Oct 5, 2017

View reviewed changes

amarie401 force-pushed the canvas-to-component branch 2 times, most recently from 6f1687c to e457ae0 Compare October 5, 2017 20:36

amarie401 force-pushed the canvas-to-component branch from e457ae0 to ea1f04c Compare October 5, 2017 23:26

dtaylor113 approved these changes Oct 6, 2017

View reviewed changes

cdcabrera requested changes Oct 9, 2017

View reviewed changes

jeff-phillips-18 reviewed Oct 9, 2017

View reviewed changes

jeff-phillips-18 requested changes Oct 9, 2017

View reviewed changes

amarie401 force-pushed the canvas-to-component branch from ea1f04c to 7397ace Compare October 9, 2017 19:21

jeff-phillips-18 reviewed Oct 9, 2017

View reviewed changes

amarie401 force-pushed the canvas-to-component branch from 7397ace to 67053e8 Compare October 9, 2017 19:24

jeff-phillips-18 reviewed Oct 9, 2017

View reviewed changes

amarie401 force-pushed the canvas-to-component branch from 67053e8 to b981aa1 Compare October 9, 2017 21:36

cdcabrera requested changes Oct 10, 2017

View reviewed changes

jeff-phillips-18 reviewed Oct 10, 2017

View reviewed changes

dtaylor113 reviewed Oct 10, 2017

View reviewed changes

amarie401 force-pushed the canvas-to-component branch from b981aa1 to 24c0f66 Compare October 10, 2017 19:21

cdcabrera reviewed Oct 11, 2017

View reviewed changes

dtaylor113 reviewed Oct 11, 2017

View reviewed changes

amarie401 force-pushed the canvas-to-component branch from 24c0f66 to 4a57c93 Compare October 11, 2017 13:55

jeff-phillips-18 approved these changes Oct 11, 2017

View reviewed changes

cdcabrera approved these changes Oct 11, 2017

View reviewed changes

amarie401 force-pushed the canvas-to-component branch from 4a57c93 to 1a15165 Compare October 11, 2017 15:11

dtaylor113 approved these changes Oct 11, 2017

View reviewed changes

fix(Canvas): Canvas directive to component implementation

b6b0f81

amarie401 force-pushed the canvas-to-component branch from 1a15165 to b6b0f81 Compare October 11, 2017 15:14

jeff-phillips-18 merged commit cbf373d into patternfly:master Oct 11, 2017


		$document.find('body').keydown(function (evt) {

		if (evt.keyCode === aKeyCode) {

fix(Canvas): Canvas directive to component implementation #640

fix(Canvas): Canvas directive to component implementation #640

Conversation

amarie401 commented Oct 5, 2017 • edited Loading

Description

dtaylor113 left a comment

Choose a reason for hiding this comment

dtaylor113 commented Oct 5, 2017

amarie401 commented Oct 5, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amarie401 commented Oct 5, 2017 • edited Loading

dtaylor113 left a comment

Choose a reason for hiding this comment

cdcabrera 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cdcabrera Oct 9, 2017 • edited Loading

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

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

amarie401 commented Oct 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amarie401 Oct 9, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cdcabrera left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amarie401 Oct 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amarie401 Oct 10, 2017 • edited Loading

Choose a reason for hiding this comment

cdcabrera Oct 10, 2017 • edited Loading

Choose a reason for hiding this comment

amarie401 Oct 10, 2017 • edited Loading

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

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

cdcabrera 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

Choose a reason for hiding this comment

amarie401 commented Oct 11, 2017

cdcabrera commented Oct 11, 2017 • edited Loading

jeff-phillips-18 commented Oct 11, 2017

cdcabrera left a comment

Choose a reason for hiding this comment

amarie401 commented Oct 11, 2017

jeff-phillips-18 commented Oct 11, 2017

dtaylor113 commented Oct 11, 2017

amarie401 commented Oct 11, 2017

jeff-phillips-18 commented Oct 11, 2017

amarie401 commented Oct 5, 2017 •

edited

Loading

amarie401 commented Oct 5, 2017 •

edited

Loading

cdcabrera Oct 9, 2017 •

edited

Loading

amarie401 Oct 9, 2017 •

edited

Loading

amarie401 Oct 10, 2017 •

edited

Loading

amarie401 Oct 10, 2017 •

edited

Loading

cdcabrera Oct 10, 2017 •

edited

Loading

amarie401 Oct 10, 2017 •

edited

Loading

cdcabrera commented Oct 11, 2017 •

edited

Loading

cdcabrera commented Oct 11, 2017 •

edited

Loading

dtaylor113 commented Oct 11, 2017 •

edited

Loading

amarie401 commented Oct 11, 2017 •

edited

Loading