feat(Filter): Add Filter Component#138
Conversation
src/components/Filter/Filter.js
Outdated
| import React from 'react'; | ||
| import PropTypes from 'prop-types'; | ||
|
|
||
| class Filter extends React.Component { |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Updated. Thanks.
ce9e80f to
716b807
Compare
| } | ||
|
|
||
| let menuId = 'filterCategoryMenu'; | ||
| menuId += id ? '_' + id : ''; |
There was a problem hiding this comment.
Maybe a template literal with backticks... think there are a couple of these
menuId += id ? `_${id}` : ...
|
@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 |
| FilterCategorySelector, | ||
| FilterCategoryValueSelector | ||
| } from '../../../index'; | ||
| import { boolean } from '@storybook/addon-knobs/dist/index'; |
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
I've removed the need for the import.
b242953 to
98f2553
Compare
98f2553 to
c585cd5
Compare
|
Updated to stateless and added intellisense for all sub-classed components |
c585cd5 to
acee09c
Compare
|
Simplified storybook stories. |
acee09c to
aadaafa
Compare
|
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... |
|
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. |
|
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. |
aadaafa to
3282d77
Compare
|
Updated to patternfly 3.35.0 and removed filter specific less/sass |
cdcabrera
left a comment
There was a problem hiding this comment.
@jeff-phillips-18 are the constants used anymore, or they left over?
3282d77 to
5c1d4b0
Compare
|
@cdcabrera correct, thanks. I've removed it. |
cdcabrera
left a comment
There was a problem hiding this comment.
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
5c1d4b0 to
1f6a603
Compare
|
yep aside from the good to merge here 😉 |
What:
Adding basic Filter component
Link to Storybook:
https://jeff-phillips-18.github.io/patternfly-react/?selectedKind=Filter&selectedStory=Filter
closes #106
FYI @serenamarie125 @ohadlevy