Remove JQuery dependency from default modules #337

jeff-phillips-18 · 2016-11-03T17:10:53Z

Description

This PR removes the JQuery dependency from the default patternfly module. Two of the existing directives have been moved from the default module to patternfly.jquery: pfSelect and pfDatepicker. To use either of these directives add the patternfly.jquery module to your application module.

An additional directive, pfBootstrapSelect, has been added to achieve the same basic features of pfSelect but without the JQuery dependency.

The angular patternfly code has been adjusted to use the non-JQuery patternfly settings. To keep JQuery independence, include only patternfly-settings.js rather than patternfly.js.

@dtaylor113 @dgutride @dlabrecq @bleathem

dtaylor113 · 2016-11-11T14:08:37Z

src/bootstrap-select/bootstrap-select.directive.js

+  'use strict';
+
+  return {
+    restrict: 'A',


Why just an 'A'? I expected <pf-bootstrap-select..., not having to have to use a <div pf-bootstrap-select=...
Not a big issue though

Typically the Angular Patternfly directives are Attribute only.

dtaylor113 · 2016-11-11T14:11:42Z

src/navigation/vertical-navigation-directive.js

 * @param {boolean} updateActiveItemsOnClick Flag if active items should be marked on click rather than on navigation change, default: false
 * @param {boolean} ignoreMobile Flag if mobile state should be ignored (use only if absolutely necessary) default: false
 *
 * @example


What are all these changes to vert. nav? Doesn't seem to have anything to do with removing jQuery or pf-select?

The example was using JQuery to initialize the vertical navigation. Things like:
$(document).ready(function()
I'm trying to use as little jQuery as possible throughout even in the examples.

Ok, thanks for the info.

dtaylor113 · 2016-11-11T14:12:32Z

src/notification/inline-notification.directive.js

-           <div class="col-sm-10">
-            <select pf-select ng-model="type" id="type" ng-options="o as o for o in types"></select>
+           <div class="col-sm-4">
+            <div pf-bootstrap-select current-selection="type" id="type" selections="types"></divselect>


Is that correct closing tag? </divselect>?

Nope. I will fix that. Thanks.

dtaylor113 · 2016-11-11T14:19:27Z

This is great work Jeff! I realize this was the result of a breakaway sprint story whose end goal was to remove jQuery dependency, so this PR contains all those changes to accomplish that goal. However, this PR changes several components. Ideally, this PR would have been better as several smaller PRs. I'm going to clone this dev branch and do some manual testing, but so far everything looks good.
Thanks,

Dave

jeff-phillips-18 · 2016-11-11T14:25:42Z

Thanks @dtaylor113. Agreed which is why I did not squash commits so that the progression was evident and each commit is reviewable on its own.

dtaylor113 · 2016-11-11T14:43:29Z

Think we need to udpate the README file with some of the stuff you talk about in this PR's Description:

"This PR removes the JQuery dependency from the default patternfly module. Two of the existing directives have been moved from the default module to patternfly.jquery: pfSelect and pfDatepicker. To use either of these directives add the patternfly.jquery module to your application module.

The angular patternfly code has been adjusted to use the non-JQuery patternfly settings. To keep JQuery independence, include only patternfly-settings.js rather than patternfly.js."

dtaylor113 · 2016-11-11T15:16:15Z

Hi @jeff-phillips-18, ran through all ngdoc examples. I'm seeing two issues:

In ngdocs:view

pfFilter input field doesn't seem to be accepting input, filter not being applied. Also seen in pfToolbar
patternfly.jquery.pfDatepicker, throws error:

angular.js:13920 Error: [$injector:modulerr] Failed to instantiate module patternfly.datepicker due to:
Error: [$injector:nomod] Module 'patternfly.datepicker' is not available!

Everything else looks good 👍

dtaylor113 · 2016-11-11T15:26:22Z

Probably not due to this PR, but pfValidation:

After clicking on the button with no label:

jeff-phillips-18 · 2016-11-11T16:02:31Z

Fixed the 3 issues noted by @dtaylor113

dtaylor113 · 2016-11-11T16:15:16Z

README.md

+          'patternfly.jquery'
+]);

+    Note: the pfSelect and datepicker directives are only available in the patternfly.jquery module. Using these directives requires the inclusion of the jQuery library.


Should 'datepicker' be 'pfDatepicker' or are you also including the straight bootstrap-datepicker as requiring jQuery?

I know base refs README talks about Wiredep. I just played around with angular-patternfly-demo-app which uses something like wiredep (grunt-usemin?) and it was always including patternfly.js. Since we talk about just using patternfly-settings.js here, should we mention how to config wiredep to grab the correct file, or is that up to the app dev to know/figure out?

I would prefer to leave that exercise to the reader. You can setup wiredep to include/exclude how you like.

jeff-phillips-18 · 2016-11-11T16:19:20Z

I haven't changed the name and would prefer not to to keep as much backwards compatibility as possible.

dtaylor113 · 2016-11-11T16:23:04Z

LGTM 👍

dtaylor113 · 2016-11-15T14:08:46Z

Any movement on this? Can we get another reviewer? @dgutride , @dlabrecq -thanks 😄

bleathem · 2016-11-15T17:39:25Z

README.md

-          'patternfly.charts'
-        ]);
+          'patternfly.charts',
+          'patternfly.jquery'


We seem to have lost the granularity of our modules. Should we consider keeping the granularity, but adding a -jquery suffix to the modules to indicate they bring in the jQuery dependency (with possibly a common jquery parent dependency)? eg. patternfly-datepicker-jquery or patternfly-form-jquery.

There are 2 jquery dependent directives. We could add modules for each patternfly.datepicker and patternfly.select then have patternfly.jquery pull both in. I wouldn't be a fan of patternfly-form-jquery with only one directive then patternfly.jquery pull in patternfly-form-jquery. that's 3 modules for one directive.

bleathem · 2016-11-15T17:43:07Z

I've reviewed each of the commits - thanks for not squashing them @jeff-phillips-18, it's much easier to review each change in context. The changes overall seem reasonable to me, I'll leave it up to those more familiar with the angular-patternfly implementation to assess there technical validity (thanks @dtaylor113).

I had one question I asked inline about the module consolidation into the module.jquery module.

The only other question I have is to confirm that this PR does not introduce a breaking change for downstream applications. Rather they will have to "opt in" to get the jQuery independence provided by this PR.

Nice work overall @jeff-phillips-18, a very well thought out and comprehensive approach!

jeff-phillips-18 · 2016-11-15T17:48:28Z

Well, this is the opposite @bleathem. Any downstream applications currently using pfSelect or pfDatepicker will need to "opt in" in order to continue using them (by adding a dependency on the patternfly.jquery module).

To "opt out" of jquery dependence would require a patternfly module that was jquery independent. I couldn't think of a good naming convention for that. Any thoughts or is release noting this good enough?

jeff-phillips-18 · 2016-11-18T13:07:18Z

@bleathem ^^

Using the hyphenated version of the require specifications does not work in a non-jquery environment.

…xample

jeff-phillips-18 force-pushed the no-jquery branch 2 times, most recently from 2b45a01 to 1c1c4a6 Compare November 4, 2016 17:56

jeff-phillips-18 changed the title ~~[WIP] Remove JQuery dependency from default modules~~ Remove JQuery dependency from default modules Nov 7, 2016

jeff-phillips-18 force-pushed the no-jquery branch from 1c1c4a6 to 4512cc4 Compare November 10, 2016 13:33

dtaylor113 reviewed Nov 11, 2016

View reviewed changes

dtaylor113 approved these changes Nov 11, 2016

View reviewed changes

jeff-phillips-18 force-pushed the no-jquery branch from 5fa2925 to 4ccbb17 Compare November 11, 2016 18:06

bleathem reviewed Nov 15, 2016

View reviewed changes

jeff-phillips-18 mentioned this pull request Nov 17, 2016

Add datetimepicker #344

Merged

lorthirk mentioned this pull request Nov 18, 2016

Require the child patternfly module instead of a sibling one #347

Merged

jeff-phillips-18 force-pushed the no-jquery branch from 4ccbb17 to 47ccc07 Compare November 18, 2016 17:53

jeff-phillips-18 added 4 commits November 18, 2016 13:04

Update to use patternfly settings without jQuery

e864e5d

Add non-jquery based bootstrap select directive

4b5fe94

Change wizard directive requires settings to be standard

e3c7f53

Using the hyphenated version of the require specifications does not work in a non-jquery environment.

Update directives to be non-jquery dependent where possible

283e4c6

jeff-phillips-18 added 4 commits November 18, 2016 13:04

Update directives to use bootstrap select rather than select

e8d27c1

Move jquery dependent directives to optional jquery module

c701f71

Update README file and fix for an ending tag in inline notification e…

f3ec32f

…xample

Fixes for filter input, date picker, and validate example layout

cdc22d7

jeff-phillips-18 force-pushed the no-jquery branch from 47ccc07 to cdc22d7 Compare November 18, 2016 18:05

jeff-phillips-18 closed this Nov 18, 2016

jeff-phillips-18 deleted the no-jquery branch March 13, 2018 11:38

Remove JQuery dependency from default modules #337

Remove JQuery dependency from default modules #337

Uh oh!

Conversation

jeff-phillips-18 commented Nov 3, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

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 Nov 11, 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!

dtaylor113 commented Nov 11, 2016

Uh oh!

jeff-phillips-18 commented Nov 11, 2016

Uh oh!

dtaylor113 commented Nov 11, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dtaylor113 commented Nov 11, 2016

Uh oh!

dtaylor113 commented Nov 11, 2016

Uh oh!

jeff-phillips-18 commented Nov 11, 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 commented Nov 11, 2016

Uh oh!

dtaylor113 commented Nov 11, 2016

Uh oh!

dtaylor113 commented Nov 15, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bleathem commented Nov 15, 2016

Uh oh!

jeff-phillips-18 commented Nov 15, 2016

Uh oh!

jeff-phillips-18 commented Nov 18, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jeff-phillips-18 commented Nov 3, 2016 •

edited

Loading

dtaylor113 Nov 11, 2016 •

edited

Loading

dtaylor113 commented Nov 11, 2016 •

edited

Loading