fix(DataToolbar): implement changes to match PF-Core by nicolethoen · Pull Request #3144 · patternfly/patternfly-react · GitHub

Implements changes made to core in this PR #2342

Codecov Report

Merging #3144 into master will decrease coverage by 0.04%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #3144      +/-   ##
==========================================
- Coverage   69.03%   68.98%   -0.05%     
==========================================
  Files         859      858       -1     
  Lines       23637    23627      -10     
  Branches     1895     1893       -2     
==========================================
- Hits        16318    16300      -18     
- Misses       6359     6366       +7     
- Partials      960      961       +1

Flag	Coverage Δ
#misc	95.45% <ø> (ø)	⬆️
#patternfly3	69.3% <ø> (ø)	⬆️
#patternfly4	67.95% <ø> (-0.11%)	⬇️

Impacted Files	Coverage Δ
...components/ClipboardCopy/ClipboardCopyExpanded.tsx	70.58% <0%> (-14.03%)	⬇️
...nfly-4/react-core/src/components/Select/Select.tsx	58.85% <0%> (-2.93%)	⬇️
...y-react-extensions/src/components/Select/Select.js	19.71% <0%> (-0.1%)	⬇️
...re/src/experimental/components/Divider/Divider.tsx
...rnfly-4/react-core/src/components/withOuia/ouia.ts	66.66% <0%> (+4.16%)	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1f0defa...f4be16f. Read the comment docs.

PatternFly-React preview: https://patternfly-react-pr-3144.surge.sh

I'm wondering if the toggle props can be simplified?

The isConsumerManagedToggleGroup and componentManagedIsExpanded names are a little confusing to me.

Perhaps something more like isToggleManaged would suffice?

I'm struggling with the componentManagedIsExpanded name. Couldn't a user use isExpanded directly, when isConsumerManagedToggleGroup is in use?

Could this be simplified at all?

I will change the names of the isConsumerManagedToggleGroup and componentManagedIsExpanded to try and clear it up some.

Basically, I allow the consumer to control the expanded state of the DataToolbarContentGroup by passing isExpanded and toggleIsExpanded as props. If they are not provided by the consumer, then both are managed internally by the component. That was what I was trying to communicate. I use the state to track whether or not the user has provided those props - thus wants to manage the expanded state of the toggle group themself.

In the react docs I do try to provide a write up explaining some of that. Maybe that clears it up some? I could add more of that into the comments of the component code for the sake of the developers in the future.

Storing isToggleManaged in state seems like an antipattern to me, since it is something derived from props. If those props were to change, the state wouldn't update. Since the component will probably never go from managed to unmanaged or vice versa while it is mounted (I don't see why a consumer would do that) it's probably fine here, but I could see this pattern causing bugs if used elsewhere. Instead maybe putting that logic in a method and calling this.isToggleManaged() wherever you need to check that would be better.

oh I see

It's difficult to understand the difference between isExpanded, toggleIsExpanded, expandableContent*, etc. It would be good to simplify / clarify these names.

Is this for the toggle group or content? The description, "toggle group's expandable content is expanded" sounds confusing to me.

Considering there is a toggleIsExpanded prop below, perhaps something more like isContentExpanded would help to clarify the use case?

so the toggle group has an associated expandable content that is what gets expanded or collapsed when the toggle icon in the toggle group is clicked. It is confusing. I feel passing an isExpanded flag as a prop to the DataToolbarToggleGroup is the clearest way to state it. Do you have another idea?

It appears that we have two sets of properties, one set managed by the consumer and another internally managed. My thought was that we could use the same props internally, if isConsumerManagedToggleGroup is not provided?

I don't feel strongly about it, just somewhat confusing. I didn't see the react docs, so clarifying the comments would be helpful.

isConsumerManagedToggleGroup is a state field calculated based on whether or not the consumer provided isExpanded and the toggleIsExpanded handler, not a passed prop. But I will rename it to isToggleManaged and make the comments clearer?

gotcha

Good to see the progress on this @nicolethoen . Just a few questions/comments:

It still looks like we are not applying the latest styling from core. I'm specifically looking at the search fields which should pick up the revised input group styling.
The way the toolbar responds as I shrink the viewport in the examples is not consistent with the design intent. See the section on Responsive toolbar here: https://www.patternfly.org/v4/design-guidelines/usage-and-behavior/toolbar. Maybe this is OK for these examples and we should build something more like a Toolbar demo that responds correctly. Not sure. Perhaps we didn't address this fully in core. @mattnolting what are your thoughts? Should we be using the Overflow component in the place of just groups of buttons?
Without more documentation, I'm not sure that the examples here have enough context. Do we need to explain the spacing system? I tried to do that in the design documentation (linked above) so maybe we just need to reference that?

In short, I think what's implemented here meets the goal to better match the core examples and I don't have a problem accepting this PR. But I just wonder if the examples will be enough on their own to give developer guidance. Thoughts @LHinson @rachael-phillips @dgutride ?

@mcarrano

It still looks like we are not applying the latest styling from core. I'm specifically looking at the search fields which should pick up the revised input group styling.

Good catch, I had to make a code change to apply the new style for inputGroup. I guess that was a change made after I first used the InputGroups in the examples.

The way the toolbar responds as I shrink the viewport in the examples is not consistent with the design intent. See the section on Responsive toolbar here: https://www.patternfly.org/v4/design-guidelines/usage-and-behavior/toolbar. Maybe this is OK for these examples and we should build something more like a Toolbar demo that responds correctly. Not sure. Perhaps we didn't address this fully in core. @mattnolting what are your thoughts? Should we be using the Overflow component in the place of just groups of buttons?

If you are referring to the groups of buttons in the first few examples. Those examples are not demonstrating the responsive behavior of the toolbar, they are demonstrating the spacing between items and groups of items and how that can be manipulated.

As far as I understand, the responsive solution in the toolbar is heavily tied to the Toggle Group, so that is not used or demonstrated until later examples. Is there something else that is missing?

Without more documentation, I'm not sure that the examples here have enough context. Do we need to explain the spacing system? I tried to do that in the design documentation (linked above) so maybe we just need to reference that?

I'm happy to add more text - when doing my own poking around, it didn't seem that the react examples tended to include a grand deal of explanation. So I assumed that we expected consumers to look elsewhere for design decision explanation and such - but I can definitely see how more text is useful to a developer. If no one sees a problem with adding more information. I'll do that.

If you are referring to the groups of buttons in the first few examples. Those examples are not demonstrating the responsive behavior of the toolbar, they are demonstrating the spacing between items and groups of items and how that can be manipulated.

Upon further thinking, I guess this is OK. Maybe this would be solved if there was some documentation that describes the purpose of those examples. I just didn't want someone to look at this and think it was the responsive behavior that we want (with everything wrapping to multi-lines).

LGTM

Looks great @nicolethoen . Thanks for adding the documentation. I think that helps a lot.

can you make this sentence case. That goes for all prop descriptions.

can you make this sentence case.

@mattnolting
does this incorporate changes necessary to consider this issue addressed?
#3046

@nicolethoen yes, .pf-m-expanded is applied to pf-c-data-toolbar__toggle when expanded. CSS is there to support

LGTM

LGTM!

Looks like there are duplicate __expandable-contents expanding at the same time..

Suggested change

      
                        <Button variant="link" onClick={clearChipGroups}>
          
                         <Button variant="link" onClick={clearChipGroups} isInline>

Toolbar height shrinks when toggle is opened.

Can you add pf-m-toggle-group-container to pf-c-data-toolbar__contents that contain __toggle-groups?

🌯 🌯

🌯 🌯 🌯

Fantastic work @nicolethoen! 👍

Don't need pf-m-align-right here: https://github.com/patternfly/patternfly-react/pull/3144/files#diff-71a8fa8b3d9c6597956a184e3718d8c1R1028

…ch PF-Core

…aToolbarContent state names

…whenever there are no chips to display

Fantastic! LGTM. Thanks for your hard work @nicolethoen!

nicolethoen requested review from dlabaj, mattnolting, mcarrano, mturley and rebeccaalpert October 15, 2019 15:27

tlabaj added PF4 labels Oct 15, 2019

tlabaj assigned jessiehuff Oct 15, 2019

nicolethoen changed the title fix(DataToolbar): implement structural and interaction changes to mat… fix(DataToolbar): implement changes to match PF-Core Oct 15, 2019

nicolethoen force-pushed the toolbar_changes branch from a2a9dbb to 51c02f0 Compare October 15, 2019 18:42

dlabrecq suggested changes Oct 16, 2019

View reviewed changes

mcarrano reviewed Oct 16, 2019

View reviewed changes

dlabrecq previously approved these changes Oct 17, 2019

View reviewed changes

mcarrano previously approved these changes Oct 17, 2019

View reviewed changes

tlabaj reviewed Oct 17, 2019

View reviewed changes

nicolethoen dismissed stale reviews from mcarrano and dlabrecq via e1b1374 October 17, 2019 18:52

nicolethoen force-pushed the toolbar_changes branch 3 times, most recently from 046f4cd to 595e376 Compare October 21, 2019 14:27

tlabaj previously approved these changes Oct 21, 2019

View reviewed changes

mcarrano previously approved these changes Oct 21, 2019

View reviewed changes

mattnolting reviewed Oct 21, 2019

View reviewed changes

dlabrecq previously approved these changes Oct 22, 2019

View reviewed changes

nicolethoen dismissed stale reviews from dlabrecq and mturley via 040dd0b October 22, 2019 19:26

mturley previously approved these changes Oct 22, 2019

View reviewed changes

nicolethoen dismissed mturley’s stale review via f4be16f October 22, 2019 20:29

nicolethoen force-pushed the toolbar_changes branch from 040dd0b to f4be16f Compare October 22, 2019 20:29

mturley previously approved these changes Oct 22, 2019

View reviewed changes

mattnolting previously approved these changes Oct 23, 2019

View reviewed changes

nicolethoen added 12 commits October 23, 2019 07:43

fix(DataToolbar): implement structural and interaction changes to mat…

41aaf9c

…ch PF-Core

update documentation in examples, input group styles, and clarify Dat…

310b4c2

…aToolbarContent state names

change some wording in example documentation

9719b3f

make prop descriptions sentence case

853bef9

fix some unintented quirks in filter example

e5de2ff

fix documentation to accommodate theming

4e2be74

fix expandable content id generation

9a05d69

fix expanded state in stacked example

61e830e

move isToggleManaged out of DataToolbar.tsx state

c54da0a

more small styling updates

666ce5e

add pf-m-toggle-group-container to examples and hide chip containers …

5dc6351

…whenever there are no chips to display

remove right-align in example

db5dd59

nicolethoen dismissed stale reviews from mattnolting and mturley via db5dd59 October 23, 2019 11:50

nicolethoen force-pushed the toolbar_changes branch from f4be16f to db5dd59 Compare October 23, 2019 11:50

mattnolting approved these changes Oct 23, 2019

View reviewed changes

dlabrecq approved these changes Oct 23, 2019

View reviewed changes

mturley approved these changes Oct 23, 2019

View reviewed changes

mturley merged commit e77c613 into patternfly:master Oct 23, 2019

This was referenced Oct 24, 2019

DataToolbar: implement structural and interaction changes to match PF-Core #3097

Closed

Add support for expanded state on data toolbar's toggle button #3046

Closed

nicolethoen deleted the toolbar_changes branch February 8, 2023 13:46

+                /** Flag indicating the if the expandable content's expanded state is consumer managed or not */
+                isConsumerManagedToggleGroup: boolean;
+                /** Flag indicating if the component managed state has expanded content or not */
+                componentManagedIsExpanded: boolean;

+                          isExpanded: isConsumerManagedToggleGroup ? isExpanded : componentManagedIsExpanded,
+                          toggleIsExpanded: isConsumerManagedToggleGroup ? toggleIsExpanded : this.toggleIsExpanded,
+                          expandableContentRef: this.expandableContentRef,
+                          expandableContentId

                 /** Content to be rendered as children of the content row */
                 children?: React.ReactNode;
+                /** Flag indicating if a Data toolbar toggle group's expandable content is expanded */
+                isExpanded?: boolean;

                 showClearFiltersButton: boolean;
-                /** Flag indicating if a Data toolbar toggle group's expandable content is expanded */
-                expandableContentIsExpanded: boolean;
+                /** Text to display in the Clear all filters button */

		@@ -16,40 +17,73 @@ export interface DataToolbarChipGroupContentProps extends React.HTMLProps<HTMLDi
		clearAllFilters?: () => void;
		/** Flag indicating that the Clear all filters button should be visible */

+                      {
+                        showClearFiltersButton && !isExpanded &&
                         <DataToolbarItem className={css(getModifier(styles, 'clear'))}>
                           <Button variant="link" onClick={clearChipGroups}>

	<Button variant="link" onClick={clearChipGroups}>
	<Button variant="link" onClick={clearChipGroups} isInline>

fix(DataToolbar): implement changes to match PF-Core by nicolethoen · Pull Request #3144 · patternfly/patternfly-react · GitHub

Uh oh!

fix(DataToolbar): implement changes to match PF-Core - #3144

fix(DataToolbar): implement changes to match PF-Core#3144mturley merged 12 commits intopatternfly:masterpatternfly/patternfly-react:masterfrom nicolethoen:toolbar_changesnicolethoen/patternfly-react:toolbar_changesCopy head branch name to clipboard

Conversation

nicolethoen commented Oct 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-io commented Oct 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

patternfly-build commented Oct 15, 2019

Uh oh!

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

dlabrecq Oct 16, 2019 • 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!

dlabrecq Oct 16, 2019 • 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!

dlabrecq Oct 16, 2019 • 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!

mcarrano left a comment

Choose a reason for hiding this comment

Uh oh!

nicolethoen commented Oct 16, 2019

Uh oh!

mcarrano commented Oct 16, 2019

Uh oh!

dlabrecq left a comment

Choose a reason for hiding this comment

Uh oh!

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

nicolethoen commented Oct 21, 2019

Uh oh!

mattnolting commented Oct 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tlabaj left a comment

Choose a reason for hiding this comment

Uh oh!

mcarrano left a comment

Choose a reason for hiding this comment

Uh oh!

mattnolting left a comment • 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!

mattnolting left a comment

fix(DataToolbar): implement changes to match PF-Core#3144
mturley merged 12 commits into
patternfly:masterfrom
nicolethoen:toolbar_changes

nicolethoen commented Oct 15, 2019 •
edited
Loading

codecov-io commented Oct 15, 2019 •
edited
Loading

dlabrecq Oct 16, 2019 •
edited
Loading

dlabrecq Oct 16, 2019 •
edited
Loading

dlabrecq Oct 16, 2019 •
edited
Loading

mattnolting commented Oct 21, 2019 •
edited
Loading

mattnolting left a comment •
edited
Loading