feat(Toolbar): Add Toolbar and its components by jeff-phillips-18 · Pull Request #150 · patternfly/patternfly-react

jeff-phillips-18 · 2018-01-04T20:35:46Z

What:
Adding basic Filter component

Links to Storybooks:
Filter: https://jeff-phillips-18.github.io/patternfly-react/?selectedKind=Filter&selectedStory=Filter
Sort: https://jeff-phillips-18.github.io/patternfly-react/?selectedKind=Sort&selectedStory=Sort
Toolbar: https://jeff-phillips-18.github.io/patternfly-react/?selectedKind=Toolbar&selectedStory=Toolbar

closes #102
closes #124

FYI @serenamarie125 @ohadlevy

priley86 · 2018-01-04T21:01:59Z

looking very promising @jeff-phillips-18 ! The recompose/context usage is very nice. I will dive in more soon!

jeff-phillips-18 · 2018-01-04T21:08:24Z

@priley86 Thanks, the recompose/context usage credit goes to @mturley !

priley86 · 2018-01-05T17:29:24Z

src/components/Sort/__mocks__/mockSortExample.js

+  toggleCurrentSortDirection() {
+    const { isSortAscending } = this.state;
+
+    this.setState({ isSortAscending: !isSortAscending });


simple toggles like this can be simplified to:

this.setState(prevState => { return { isSortAscending: !prevState.isSortAscending }; });

as setState also accepts a function ;)
https://medium.com/@shopsifter/using-a-function-in-setstate-instead-of-an-object-1f5cfd6e55d1

priley86 · 2018-01-05T17:34:34Z

src/components/Sort/__mocks__/mockSortExample.js

+import React from 'react';
+import { Sort, Toolbar } from '../../../index';
+
+const bindMethods = (context, methods) => {


should just be able to:
import { bindMethods } from '../../../common/helpers';

it seems to work here...

priley86 · 2018-01-05T17:37:43Z

src/components/Filter/FilterActiveLabel.js

+  /** Additional css classes */
+  className: PropTypes.string,
+  /** Title for the list (ie. 'Active Filters:') */
+  title: PropTypes.string.isRequired


I know that {title} is typically just a string... but {children} is more flexible. What if I want to put an emoji in there and a <b> tag... 😸

another option is to just use PropTypes.node and leave it named {title}...

title is bad anyhow, causes unwanted tooltips at times, I will update

priley86 · 2018-01-05T17:42:10Z

src/components/Filter/FilterItem.js

+  /** additional filter item classes */
+  className: PropTypes.string,
+  /** Label to show for the filter item */
+  label: PropTypes.string.isRequired,


same as above... PropTypes.node preferred for label

priley86 · 2018-01-05T17:44:06Z

src/components/Filter/__mocks__/mockFilterExample.js

-import { Grid, Col, Row, Filter } from '../../../index';
+import { Filter, Toolbar } from '../../../index';

 const bindMethods = (context, methods) => {


same with: import { bindMethods } from '../../../common/helpers';

priley86 · 2018-01-05T17:53:59Z

src/components/Filter/__mocks__/mockFilterExample.js

+    this.setState({ activeFilters: activeFilters });
  };

  selectFilterType(filterType) {


I missed this last time... but it's best to try to combine calls to setState when possible... as every call to setState forces a re-evaluation of the virtual dom in render() method. Maybe something like this can work here:

this.setState(prevState => { return { currentValue: '', currentFilterType: filterType, filterCategory: filterType.filterType === 'complex-select' ? undefined : prevState.filterCategory, categoryValue: filterType.filterType === 'complex-select' ? '' : prevState.categoryValue }; });

Not a big deal... just more of an awareness thing ;)

Thanks. Fixed.

priley86 · 2018-01-05T17:58:40Z

src/components/Toolbar/ToolbarFind.js

+
+  toggleDropdownShown() {
+    const { dropdownShown } = this.state;
+    this.setState({ dropdownShown: !dropdownShown });


same deal w/ toggles, you can use => prevState function

priley86 · 2018-01-05T17:59:58Z

src/components/Toolbar/ToolbarFind.js

+
+    if (currentValue && currentValue !== '') {
+      return [
+        <span className="find-pf-nums">


maybe the span and buttons can be combined into another stateless component ? Not sure how common it is...

I'd prefer to keep it all self contained unless you feel strongly.

fine w/ me, just wasn't sure how common. ;)

priley86 · 2018-01-05T18:00:39Z

As a first pass...very nice job with this @jeff-phillips-18 ! Also the ContextProvider pattern is beautiful here 👍

cdcabrera · 2018-01-05T19:51:46Z

src/components/Filter/FilterItem.js

+import PropTypes from 'prop-types';
+
+const FilterItem = ({ className, label, onRemove, filterData, ...rest }) => {
+  const classes = cx(className);


For some reason I keep coming back to this bit...

FilterItem = ({ className, label, onRemove, filterData, ...rest })

Not sure if we do this anywhere else within this repo, but label is a property on filterdata is that something we could combine to avoid passing the same data in while relating some of the context? Though saying that now I'm thinking we'd lose some flexibility.

Yeah, we would have to force the filterData structure. And the label property is being removed in favor of children to allow users to show more than just a label here.

priley86 · 2018-01-09T14:28:18Z

src/components/Filter/Filter.test.js

+          filterTypes={mockFilterExampleFields}
+          currentFilterType={mockFilterExampleFields[0]}
+        />
+        <input


we can use FormControl instead of inputs now that #131 has landed.

priley86 · 2018-01-09T14:39:25Z

src/components/Toolbar/ToolbarConstants.js

@@ -0,0 +1,20 @@
+import React from 'react';


can we rename this file to simply constants.js to match the other components?

I actually find it confusing to have just constants.js which contain component specific constants.

ok... no issues there then. I guess this is a bit more specific (as they are only specific to the Toolbar and not other sub-toolbar components). I guess typically I have been including those in the component itself in those cases, but fine with this convention. Is this the correct understanding? Or are you proposing that we rename all constants.js files?

I just like to have the file named for what it is so if I have multiple open in my IDE I can see which is the Toolbar constants file vs the Button constants file.

One could say, like a "constant" reminder...

I would agree with this. So if it's the global change, I think we leave it ToolbarConstants.js and then apply the same to others in a separate PR... I just wanted to make sure I understood ;)

cdcabrera · 2018-01-09T14:38:19Z

src/components/Toolbar/ToolbarViewSelector.js

+import cx from 'classnames';
+
+const ToolbarViewSelector = ({ children, className, ...rest }) => {
+  const classes = cx('form-group toolbar-pf-view-selector', className);


do we want the ...rest passed through to the div?

yes, added. thanks.

cdcabrera · 2018-01-09T14:38:55Z

src/components/Toolbar/ToolbarRightContent.js

+import cx from 'classnames';
+
+const ToolbarRightContent = ({ children, className, ...rest }) => {
+  const classes = cx('toolbar-pf-action-right', className);


do we want the ...rest passed through to the div?

yes, added. thanks.

priley86 · 2018-01-09T14:45:55Z

the updates look good to me here... was chatting a bit with @cdcabrera, if you have extra time, it might be nice to pass down actions as props into the Filters example and log the filter values as they change in the storybook Actions console. I think it just helps clarify the examples more for developers but is definitely a "nice-to-have".

So something like this is the way to pass in actions Filter.stories.js...
priley86@1be9253#diff-af9477690520614260c8b88ebe909580

edit: updated example with decoratorAction
priley86@a914026

jeff-phillips-18 · 2018-01-09T18:42:55Z

Updated per review comments. Added logging of actions into the toolbar action logger

jeff-phillips-18 · 2018-01-09T19:15:59Z

Added more logging of actions in the toolbar storybook

jeff-phillips-18 · 2018-01-09T19:45:00Z

Rebased

jeff-phillips-18 requested review from cdcabrera and priley86 January 4, 2018 20:35

jgiardino added the review label Jan 4, 2018

priley86 reviewed Jan 5, 2018

View reviewed changes

cdcabrera reviewed Jan 5, 2018

View reviewed changes

jeff-phillips-18 force-pushed the toolbar branch from 60fdce1 to 0fcaf92 Compare January 8, 2018 12:23

priley86 reviewed Jan 9, 2018

View reviewed changes

cdcabrera reviewed Jan 9, 2018

View reviewed changes

jeff-phillips-18 force-pushed the toolbar branch from 0fcaf92 to 684ca8c Compare January 9, 2018 18:41

jeff-phillips-18 added 5 commits January 9, 2018 14:40

feat(Toolbar): Add Toolbar and its components

888b768

remove extraneous span wrapper from ToolbarFind

ba03103

Updates per review comments

1c1b780

More updates per review comments

175e3ef

Add more action logging for toolbar

6f48c8d

jeff-phillips-18 force-pushed the toolbar branch from 1493bc0 to 6f48c8d Compare January 9, 2018 19:44

cdcabrera approved these changes Jan 9, 2018

View reviewed changes

priley86 merged commit 77fcdb1 into patternfly:master Jan 9, 2018

priley86 mentioned this pull request Jan 10, 2018

Rename constants.js files for consistency #159

Closed

aljesusg mentioned this pull request Feb 15, 2018

Issue #159 fix(constants file): Rename all constants file to {Component_Name}Con… #228

Merged

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

Conversation

jeff-phillips-18 commented Jan 4, 2018

Uh oh!

priley86 commented Jan 4, 2018

Uh oh!

jeff-phillips-18 commented Jan 4, 2018

Uh oh!

priley86 Jan 5, 2018 • 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!

priley86 Jan 5, 2018 • 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!

priley86 Jan 5, 2018 • 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!

priley86 Jan 5, 2018 • 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!

Choose a reason for hiding this comment

Uh oh!

priley86 commented Jan 5, 2018

Uh oh!

cdcabrera Jan 5, 2018 • 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!

priley86 Jan 9, 2018 • 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!

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!

priley86 commented Jan 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jeff-phillips-18 commented Jan 9, 2018

Uh oh!

jeff-phillips-18 commented Jan 9, 2018

Uh oh!

priley86 Jan 5, 2018 •

edited

Loading

priley86 Jan 5, 2018 •

edited

Loading

priley86 Jan 5, 2018 •

edited

Loading

priley86 Jan 5, 2018 •

edited

Loading

cdcabrera Jan 5, 2018 •

edited

Loading

priley86 Jan 9, 2018 •

edited

Loading

priley86 commented Jan 9, 2018 •

edited

Loading