PatternFly Angular wizard implementation #320

dgutride · 2016-09-27T02:38:25Z

This PR includes the initial implementation, documentation and unit tests for the angular-patternfly wizard.

…support

…-hand navigation behavior

bleathem · 2016-09-27T18:53:07Z

Thanks @dgutride, I'll take a close look. Probably best if @jeff-phillips-18, @dlabrecq, and @dtaylor113 take a look too.

dtaylor113 · 2016-09-27T19:34:19Z

bower.json

@@ -39,9 +42,17 @@
    "angular-sanitize": "1.3.0 - 1.5.*",
    "angular-bootstrap": "0.13.x",
    "lodash": "3.x",
-    "patternfly": "~3.11.0"
+    "patternfly": "a014002ebcfad2c6ff2fbeda52dfcd425e016f2a"


Is this the pattern now?! PF devs know to change this back to a version number for the next patternfly release? Any reason why this couldn't be "git@github.com:patternfly/patternfly.git#master" -just to be more readable.

We can't use #master, but I changed this to master-dist for clarity instead of using the commit.

coolness, thanks

jeff-phillips-18

Looks really good. Just some minor comments

jeff-phillips-18 · 2016-09-27T19:10:14Z

bower.json

@@ -39,9 +42,17 @@
    "angular-sanitize": "1.3.0 - 1.5.*",
    "angular-bootstrap": "0.13.x",
    "lodash": "3.x",
-    "patternfly": "~3.11.0"
+    "patternfly": "a014002ebcfad2c6ff2fbeda52dfcd425e016f2a"


So this is WIP? I assume this will require patternfly 3.12?

Correct - cleaned this up a bit with the last commit.

jeff-phillips-18 · 2016-09-27T19:10:25Z

bower.json

+    "bootstrap-touchspin": "~3.1.1",
+    "datatables-colreorder": "~1.3.2",
+    "font-awesome": "~4.6.3",
+    "moment": "~2.14.1"
  }


I assume this all goes away when we update to the correct version of patternfly

Removed it now.

jeff-phillips-18 · 2016-09-27T19:10:30Z

src/wizard/wizard-directive.js

+  *
+  * @description
+  * Directive for rendering a Wizard modal.
+  *


I think more information is needed here. The overall structure of the wizard directive, step directive, and sub step directives.

More details and an html snippet added to clarify the structure.

jeff-phillips-18 · 2016-09-27T19:42:13Z

src/wizard/wizard-step-directive.js

+ *
+ * @description
+ * Directive for rendering a Wizard step.  Each step can stand alone or have substeps.
+ *


Indicate that a pfWizardStep must be a child of a pfWizard

jeff-phillips-18 · 2016-09-27T19:46:41Z

test/wizard/wizard.spec.js

+    var closeButton = element.find('.close');
+    $rootScope.$on('wizard.done', function (event, data) {
+      expect(data).toBe('cancel');
+    });


I don't see that we check that this was called, only that if it was called, the data is correct.

You are correct - I removed the .$on portion and added a better emit check lower to make sure the data is correct.

jeff-phillips-18 · 2016-09-27T19:51:36Z

test/wizard/wizard.spec.js

+    $scope.currentStep = 'Review';
+    $scope.$digest();
+    $timeout.flush();
+    $timeout.flush();


Is this all necessary?

Yes - there are several layers that need to be rendered and timeouts that cascade - it doesn't show to the naked eye when rendering but they need to be flushed when in a unit test.

jeff-phillips-18 · 2016-09-27T19:52:58Z

src/wizard/wizard-directive.js

+        $element.height($window.innerHeight - 70);
+      } else {
+        $element.height(400);
+      }


400 comes from where? It would be nice if this value were settable. Some wizards could be smaller, some could be larger depending upon the contents of the pages.

This value could be binded to in the html rather than using $element.height()

Modified this logic to use a minHeight optional wizard property now. It needs to adjust on window resize so I left it in the link function.

dtaylor113 · 2016-09-27T20:21:48Z

src/wizard/wizard-directive.js

+        wizardDoneListener = $rootScope.$on('wizard.done', closeWizard);
+      };
+    }
+  ]);


I think a good task for the next breakaway sprint would be to see if we could somehow separate out the ngdoc code from the directive code. When combined it makes for a very large source code file.

…wizard directives (optional values, information about proper structure) and added minHeight property as well.

dtaylor113 · 2016-09-28T12:47:34Z

src/wizard/wizard-directive.js

+  return {
+    restrict: 'A',
+    transclude: true,
+    replace: true,


directive replace is depreciated. Do we really need it here?

dtaylor113 · 2016-09-28T12:49:06Z

src/wizard/wizard-directive.js

+        // Add watchers for the selected step
+        $scope.nextStepEnabledWatcher = $scope.$watch('selectedStep.nextEnabled', function (value) {
+          $scope.nextEnabled = value;
+          //console.log('next enabled: ' + value);


Just FYI, best practice to use $log, in this case $log.debug.

dtaylor113

Wow, tons of code! LGTM. Nice job Dana :-)

jeff-phillips-18 · 2016-09-28T14:28:14Z

src/wizard/wizard-directive.js

  * @param {string} loadingWizardTitle The text displayed when the wizard is loading
-  * @param {string} loadingSecondaryInformation Secondary descriptive information to display when the wizard is loading
+  * @param {string=} loadingSecondaryInformation Secondary descriptive information to display when the wizard is loading
+  * @param {number=} minHeight The minimum height the wizard should adjust to if the window height is decreased.  The wizard height is adjusted with the window resize event and this sets the minimum.


I was thinking more of a height than a minimum height. If all the pages have only a couple of fields, there is no reason to have the wizard be the full height of the window, making the navigation buttons far from the fields. So, maybe a minimum height and a maximum height? or a set height?

@jeff-phillips-18 - I did it this way to keep it in line with the design and js from PatternFly which keeps the height at a minHeight or within 70 pixels of the page. We'd probably have to go back to the designer to adjust it at this point or we'd be inconsistent between the two.

😞

Does the design specifically call this out or is this just how the Patternfly implementation was done?

Good question - I can ask that during the review with the stakeholders - there was a concern about the height being variable that was addressed by the last PF checkin. It was the reason I paid particular attention to the height in the code.

Agreed about the variable height. The height should not change when navigating through pages, but the height used should be configurable imo. 👍 to getting stakeholders input.

@LHinson @serenamarie125

@dgutride @jeff-phillips-18 I agree that the height should be configurable.

…erty instead of growing to the window height.

dgutride · 2016-10-04T13:01:21Z

@jeff-phillips-18 - I think I've addressed all of the comments in this review. Please let me know if there is anything else to do.

dgutride added 2 commits September 26, 2016 21:23

Add patternfly-wizard implementation - code, documentation and build …

cf3eec0

…support

Add wizard tests to cover navigation, and expected indicator and left…

a790459

…-hand navigation behavior

jeff-phillips-18 assigned bleathem, jeff-phillips-18, dtaylor113 and dlabrecq Sep 27, 2016

dtaylor113 reviewed Sep 27, 2016

View reviewed changes

jeff-phillips-18 reviewed Sep 27, 2016

View reviewed changes

dtaylor113 reviewed Sep 27, 2016

View reviewed changes

Modify bower.json to clean up reference, add better documentation to …

a594b8d

…wizard directives (optional values, information about proper structure) and added minHeight property as well.

dtaylor113 reviewed Sep 28, 2016

View reviewed changes

Change to http reference for patternfly

1f45804

dtaylor113 reviewed Sep 28, 2016

View reviewed changes

dtaylor113 approved these changes Sep 28, 2016

View reviewed changes

jeff-phillips-18 reviewed Sep 28, 2016

View reviewed changes

dgutride added 3 commits September 28, 2016 10:38

Remove replace attribute from directives (and fix unit test that broke).

f9f1932

Allow for the user to set the wizard height as an optional property

02f15e8

Modify wizard height calculation to use adjustable contentheight prop…

8e5ea0e

…erty instead of growing to the window height.

jeff-phillips-18 approved these changes Oct 4, 2016

View reviewed changes

jeff-phillips-18 merged commit 4cd43fb into patternfly:master Oct 4, 2016

dgutride deleted the angular-wizard branch December 9, 2016 16:59

PatternFly Angular wizard implementation #320

PatternFly Angular wizard implementation #320

Uh oh!

Conversation

dgutride commented Sep 27, 2016

Uh oh!

bleathem commented Sep 27, 2016

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 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!

dtaylor113 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dgutride Sep 28, 2016 • 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!

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!

dgutride commented Oct 4, 2016

Uh oh!

Uh oh!

dgutride Sep 28, 2016 •

edited

Loading