pfFilterPanel #540

dtaylor113 · 2017-08-01T15:14:48Z

Description

Filter Panel with Filter Result Tags which may contain multiple values:

Here is the ngDoc. I recommend reading the ngDoc descriptions before reviewing the code -it explains many of the functions and parameters.
I was able to post the ngDoc example by creating a new branch off of this PR branch; then in .gitignore I removed the '/doc' rule, then pushed the new ngDoc branch with all of the /doc files. (Note the rawgit url contains 'pfFilterPanelDocs' branch name).

JIRA: https://patternfly.atlassian.net/browse/OSUX-108
@serenamarie125

PR Checklist

Unit tests are included
Screenshots are attached (if there are visual changes in the UI)
A Designer (@beanh66) is assigned as a reviewer
A CSS rep (@cshinn) is assigned as a reviewer
Link to example/ngDoc

serenamarie125 · 2017-08-02T11:25:29Z

@dtaylor113 this looks great!

Just checking that this implementation will not restrict what we need in OpenShift. There we will need the keyword search to match multiple fields associated with the item ( name, description, label, etc ). I'm pretty sure that you already mentioned it's up to the developer using the filter panel, but wanted to verify.

+1 to allowing us to see ngDocs and run the example without having to build 😃 👍

jeff-phillips-18 · 2017-08-02T11:27:27Z

src/filters/examples/filter-panel.js

+ * @description
+ *   The Filter Panel is opened and closed by clicking on the Filter button.  It can contain any HTML desired by
+ *   the end user.  As such, the end user is repsonsible for constructing the <code>appliedFilters</code> array which is passed to
+ *   the filter results tags.


"end user" should probably say "application" or "application developer"

Good suggestion, Fixed, thanks

jeff-phillips-18 · 2017-08-02T11:31:50Z

src/filters/examples/filter-panel.js

+            updateFilterPanelModel(changedFilterId, changedFilterValue);
+          } else {
+            // the 'Clear All Filters' link was clicked
+            resetFilterPanelModel();


Not sure I like this being stated as the way to determine that clear all was clicked. The appliedFilters list would be empty, that is all that should need to be known.

It they click on the 'x' for all the values of all filter, then the appliedFilters list would be empty, but they didn't click on the clear all. I could program it that way, but it's less efficient in the scenario I just mentioned because clicking on the last 'x' would trigger resetFilterPanelModel() which goes through the entire model (as opposed to just clearing the last filter value whose 'x' was clicked)

jeff-phillips-18 · 2017-08-02T11:43:37Z

src/filters/examples/filter-panel.js

+          var matches = true;
+
+          _.forEach(filters, function(filter) {
+              if (!matchesFilter(item, filter)) {


Nit: would be nice to be consistent in the forEach use, either lodash or array.forEach

It would have to be lodash because we need to break out of these loops and you can't break out of an array.forEach.

just gonna leave this here

Fixed, switch to lodash _forEach.

jeff-phillips-18 · 2017-08-02T11:46:11Z

src/filters/filter-panel/filter-panel-results.html

+  <span class="toolbar-pf-results">
+    <h5>
+      {{$ctrl.config.resultsCount}}
+      <span ng-if="$ctrl.config.appliedFilters.length > 0"> of {{$ctrl.config.totalCount}}</span>


nit: Could remove the '> 0'

jeff-phillips-18 · 2017-08-02T11:50:47Z

src/filters/filter-panel/filter-panel-results.html

+          <ul class="list-inline category-values">
+            <li ng-repeat="value in filter.values">
+              <span class="active-filter label label-info">{{value}}
+                <a><span ng-click="$ctrl.clearFilter($parent.$index, $index)" class="pficon pficon-close"></span></a>


Why not just use filter and value here and not then need to look them up in the controller?

Yeah, got a little crazy here, a few rounds of implementation.
Fixed, removed the use of indexes.

jeff-phillips-18 · 2017-08-02T11:54:59Z

src/filters/filters.less

+  }
+}
+
+.label-category {


Seems too generic to me, how about pf-filter-label-category ?

jeff-phillips-18 · 2017-08-02T11:55:12Z

src/filters/filters.less

+    }
+  }
+  .list-inline > li {
+    padding-right: 0px;


remove the px

jeff-phillips-18 · 2017-08-02T11:55:49Z

src/filters/filters.less

+
+      .category-option-label {
+        vertical-align: text-bottom;
+        font-size: 12px;


jeff-phillips-18 · 2017-08-02T11:56:09Z

src/filters/filters.less

+        background-color: @color-pf-blue-500;
+        font-weight: 700;
+        padding-right: 2px;
+        padding-left: 0px;


jeff-phillips-18 · 2017-08-02T12:13:36Z

src/filters/filters.less

+    padding-right: 0px;
+    @media(max-width: @screen-md-min) {
+      margin-bottom: 3px;
+    }


Played with the responsiveness a bunch, there are some issues with the patternfly implementation here and the general layout. I don't think adding the 3px bottom margin does enough to fix this to warrant the need to include bootstrap's variables.less file. I worry that adding that could cause issues for applications pulling in the less files to build (as far as path and overrides and such).

Ok, but not sure what you are suggesting? Do more to resolve the responsiveness, remove the bootstrap variables.less usage, perhaps add our own @pf-screen-md-min, etc..?

I'm suggesting removing the @media setting all together. It doesn't do enough to help with responsiveness to warrant having it and adding in bootstrap's less file dependency.

Ok, removed the import of bootstrap variables.less and the use of @media.
Filed Issue #543 to track.

beanh66 · 2017-08-02T12:40:25Z

This looks great @dtaylor113!

dtaylor113 · 2017-08-02T13:29:52Z

it's up to the developer using the filter panel, but wanted to verify.

Yes, the app. developer has complete control on how they want to apply the filters.

cshinn · 2017-08-02T14:41:34Z

@dtaylor113 lgtm

jeff-phillips-18

LGTM, thanks @dtaylor113

cdcabrera

LGTM, some minor nits related to semicolons and left over comment but overall... kudos!

cdcabrera · 2017-08-02T15:12:26Z

src/filters/examples/filter-panel.js

+        // called when filter is changed in the filter panel
+        $scope.filterChanged = function() {
+          applyFilters();
+        }


Nit, missing semicolon..

Thanks, fixed

cdcabrera · 2017-08-02T15:12:35Z

src/filters/examples/filter-panel.js

+            delete $scope.filterPanelModel[0].value;
+            applyFilters();
+          }
+        }


Nit, missing semicolon..

cdcabrera · 2017-08-02T15:12:45Z

src/filters/examples/filter-panel.js

+                    title: filter.title,
+                    values: values
+                 };
+        }


Nit, missing semicolon..

cdcabrera · 2017-08-02T15:14:08Z

src/filters/filter-panel/filter-panel-results-component.js

+      // do a deep compare on config
+      if (!angular.equals(ctrl.config, prevConfig)) {
+//        setupConfig();
+      }


Nit, leftover comment?

Yes, leftover, removed/fixed.

dtaylor113 · 2017-08-02T15:38:14Z

I believe I've addressed all review issues. Squashed commits. Thanks for the review comments :-)

dtaylor113 requested review from beanh66, cdcabrera, cshinn and jeff-phillips-18 August 1, 2017 15:14

dtaylor113 force-pushed the pfFilterPanel branch from ec3d9f5 to 09a981e Compare August 1, 2017 15:18

serenamarie125 mentioned this pull request Aug 2, 2017

Adds complex filter select #536

Merged

jeff-phillips-18 requested changes Aug 2, 2017

View reviewed changes

cshinn approved these changes Aug 2, 2017

View reviewed changes

dtaylor113 force-pushed the pfFilterPanel branch from ad83af1 to 842ab1c Compare August 2, 2017 14:59

jeff-phillips-18 approved these changes Aug 2, 2017

View reviewed changes

cdcabrera reviewed Aug 2, 2017

View reviewed changes

pfFilterPanel

7cdf406

dtaylor113 force-pushed the pfFilterPanel branch from 842ab1c to 7cdf406 Compare August 2, 2017 15:34

jeff-phillips-18 merged commit 9742960 into patternfly:master Aug 2, 2017

dtaylor113 deleted the pfFilterPanel branch October 19, 2017 18:29

pfFilterPanel #540

pfFilterPanel #540

Uh oh!

Conversation

dtaylor113 commented Aug 1, 2017

Description

PR Checklist

Uh oh!

serenamarie125 commented Aug 2, 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!

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!

beanh66 commented Aug 2, 2017

Uh oh!

dtaylor113 commented Aug 2, 2017

Uh oh!

cshinn commented Aug 2, 2017

Uh oh!

jeff-phillips-18 left a comment

Choose a reason for hiding this comment

Uh oh!

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