feat(Filter): Add Filter Component #138

jeff-phillips-18 · 2017-12-20T17:08:01Z

What:
Adding basic Filter component

Link to Storybook:
https://jeff-phillips-18.github.io/patternfly-react/?selectedKind=Filter&selectedStory=Filter

closes #106

priley86 · 2017-12-20T17:25:42Z

src/components/Filter/Filter.js

+import React from 'react';
+import PropTypes from 'prop-types';
+
+class Filter extends React.Component {


It's stateless now 😄 ...so I think we can prefer that syntax:
https://hackernoon.com/react-stateless-functional-components-nine-wins-you-might-have-overlooked-997b0d933dbc

ex:
https://github.com/patternfly/patternfly-react/blob/master/src/components/Wizard/WizardSidebar.js

Updated. Thanks.

cdcabrera · 2017-12-20T19:51:20Z

src/components/Filter/FilterValueSelector.js

+      }
+
+      let menuId = 'filterCategoryMenu';
+      menuId += id ? '_' + id : '';


Maybe a template literal with backticks... think there are a couple of these

menuId += id ? `_${id}` : ...

Fixed. Thanks.

priley86 · 2017-12-20T20:00:33Z

@jeff-phillips-18 much closer here...

Here is a few suggested changes: priley86@4d9ab63

I am not positive, but you may be able to convert them all to stateless if just using conditional rendering inline vs event handlers on a class (seems it works OK but please test). Reason being here, those event handlers actually cause a minor degradation in performance. https://javascriptplayground.com/blog/2017/03/functional-stateless-components-react/

Also... it would nice to have the added intellisense for sub-classed components (added example there).

Nice job! I think this refactor is much more clear from a consumer perspective, however it is still a bit opinionated as far as how the logic works (not used to typically seeing this much logic in some of the components here, but this one is a bit "higher level" though). This could still be a good generalization though depending on how much you see it being used in downstream apps. Many of inner most components are now using our more basic elements and I think it is nice to have the Filter component for rendering those PF classes/divs. This is a good example of a "higher order" complex component for handling multiple form fields in a stateless manner.

priley86 · 2017-12-20T20:02:43Z

src/components/Filter/__mocks__/mockFilterExample.js

+  FilterCategorySelector,
+  FilterCategoryValueSelector
+} from '../../../index';
+import { boolean } from '@storybook/addon-knobs/dist/index';


This is throwing an error for me in the console...

Suggestion is to move the boolean up to the Filter.stories.js file and pass it down as a prop to the MockFilterExample. I tend to define all the Knobs on the *.stories.js file, but that is just preference...makes it easier to copy/paste those imports...

I've removed the need for the import.

jeff-phillips-18 · 2018-01-02T13:52:48Z

Updated to stateless and added intellisense for all sub-classed components

jeff-phillips-18 · 2018-01-02T21:20:57Z

Simplified storybook stories.

priley86 · 2018-01-04T16:13:42Z

this looks to good me. I am assuming the next step is to test integrating this w/ data tables/list view/etc. For data tables, we should be able to search the rows in the resolved rows/recompose methods (just how we did sort/paging). Adding the filters should work but will have to test it...

jeff-phillips-18 · 2018-01-04T16:21:15Z

Yeah, this will need to be integrated into the views by the application. We can add controlled versions of list view, data tables, etc as we see fit.

jeff-phillips-18 · 2018-01-04T16:22:12Z

The less/sass files here will not be needed as these selectors have been added to PF core. I will update the PF version and test, then update this PR.

jeff-phillips-18 · 2018-01-04T17:04:25Z

Updated to patternfly 3.35.0 and removed filter specific less/sass

cdcabrera

@jeff-phillips-18 are the constants used anymore, or they left over?

jeff-phillips-18 · 2018-01-04T18:23:41Z

@cdcabrera correct, thanks. I've removed it.

cdcabrera

Aside from a minor rebase, and a bit of repetition, something we can improve upon down the road looking good.

I'm up for iterative changes on this

priley86 · 2018-01-04T19:02:42Z

yep aside from the inputs I'm fine with this and would like to test integration. We can rebase those with <FormControl type="text" later on after #131 is in.

good to merge here 😉

jeff-phillips-18 requested review from cdcabrera and priley86 December 20, 2017 17:08

jgiardino added the review label Dec 20, 2017

priley86 reviewed Dec 20, 2017

View reviewed changes

jeff-phillips-18 force-pushed the filter branch from ce9e80f to 716b807 Compare December 20, 2017 18:48

cdcabrera requested changes Dec 20, 2017

View reviewed changes

priley86 reviewed Dec 20, 2017

View reviewed changes

jeff-phillips-18 force-pushed the filter branch 2 times, most recently from b242953 to 98f2553 Compare December 20, 2017 22:11

jeff-phillips-18 force-pushed the filter branch from 98f2553 to c585cd5 Compare January 2, 2018 13:51

jeff-phillips-18 force-pushed the filter branch from c585cd5 to acee09c Compare January 2, 2018 21:20

jeff-phillips-18 force-pushed the filter branch from acee09c to aadaafa Compare January 3, 2018 14:06

jeff-phillips-18 added the Do Not Merge label Jan 4, 2018

jeff-phillips-18 force-pushed the filter branch from aadaafa to 3282d77 Compare January 4, 2018 17:03

jeff-phillips-18 removed the Do Not Merge label Jan 4, 2018

cdcabrera reviewed Jan 4, 2018

View reviewed changes

jeff-phillips-18 force-pushed the filter branch from 3282d77 to 5c1d4b0 Compare January 4, 2018 18:23

cdcabrera previously approved these changes Jan 4, 2018

View reviewed changes

feat(Filter): Add Filter Component

1f6a603

jeff-phillips-18 dismissed cdcabrera’s stale review via 1f6a603 January 4, 2018 18:43

jeff-phillips-18 force-pushed the filter branch from 5c1d4b0 to 1f6a603 Compare January 4, 2018 18:43

cdcabrera approved these changes Jan 4, 2018

View reviewed changes

priley86 approved these changes Jan 4, 2018

View reviewed changes

priley86 merged commit 71af8ae into patternfly:master Jan 4, 2018

jeff-phillips-18 deleted the filter branch March 13, 2018 11:34

feat(Filter): Add Filter Component #138

feat(Filter): Add Filter Component #138

Uh oh!

Conversation

jeff-phillips-18 commented Dec 20, 2017

Uh oh!

priley86 Dec 20, 2017

Choose a reason for hiding this comment

Uh oh!

jeff-phillips-18 Dec 20, 2017

Choose a reason for hiding this comment

Uh oh!

cdcabrera Dec 20, 2017

Choose a reason for hiding this comment

Uh oh!

jeff-phillips-18 Dec 20, 2017

Choose a reason for hiding this comment

Uh oh!

priley86 commented Dec 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

priley86 Dec 20, 2017

Choose a reason for hiding this comment

Uh oh!

jeff-phillips-18 Dec 20, 2017

Choose a reason for hiding this comment

Uh oh!

jeff-phillips-18 commented Jan 2, 2018

Uh oh!

jeff-phillips-18 commented Jan 2, 2018

Uh oh!

priley86 commented Jan 4, 2018

Uh oh!

jeff-phillips-18 commented Jan 4, 2018

Uh oh!

jeff-phillips-18 commented Jan 4, 2018

Uh oh!

jeff-phillips-18 commented Jan 4, 2018

Uh oh!

cdcabrera left a comment

Choose a reason for hiding this comment

Uh oh!

jeff-phillips-18 commented Jan 4, 2018

Uh oh!

cdcabrera left a comment

Choose a reason for hiding this comment

Uh oh!

priley86 commented Jan 4, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

priley86 commented Dec 20, 2017 •

edited

Loading