Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 12 commits into from Oct 23, 2019

Conversation

@nicolethoen
Copy link
Contributor

nicolethoen commented Oct 15, 2019

Addresses issue #3097
Addresses #3046

Implements changes made to core in this PR #2342

@nicolethoen 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 nicolethoen force-pushed the nicolethoen:toolbar_changes branch from a2a9dbb to 51c02f0 Oct 15, 2019
@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Oct 15, 2019

Codecov Report

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

Impacted file tree graph

@@            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-build

This comment has been minimized.

Copy link
Contributor

patternfly-build commented Oct 15, 2019

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

Copy link
Member

dlabrecq left a comment

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

/** 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;

This comment has been minimized.

Copy link
@dlabrecq

dlabrecq Oct 16, 2019

Member

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

Perhaps something more like isToggleManaged would suffice?

This comment has been minimized.

Copy link
@dlabrecq

dlabrecq Oct 16, 2019

Member

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?

This comment has been minimized.

Copy link
@nicolethoen

nicolethoen Oct 16, 2019

Author Contributor

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.

This comment has been minimized.

Copy link
@mturley

mturley Oct 21, 2019

Collaborator

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.

This comment has been minimized.

Copy link
@nicolethoen

nicolethoen Oct 21, 2019

Author Contributor

oh I see

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

This comment has been minimized.

Copy link
@dlabrecq

dlabrecq Oct 16, 2019

Member

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

@@ -12,17 +13,107 @@ export interface DataToolbarContentProps extends React.HTMLProps<HTMLDivElement>
breakpointMods?: DataToolbarBreakpointMod[];
/** 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;

This comment has been minimized.

Copy link
@dlabrecq

dlabrecq Oct 16, 2019

Member

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?

This comment has been minimized.

Copy link
@nicolethoen

nicolethoen Oct 16, 2019

Author Contributor

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?

This comment has been minimized.

Copy link
@dlabrecq

dlabrecq Oct 16, 2019

Member

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.

This comment has been minimized.

Copy link
@nicolethoen

nicolethoen Oct 16, 2019

Author Contributor

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?

This comment has been minimized.

Copy link
@dlabrecq

dlabrecq Oct 16, 2019

Member

gotcha

Copy link
Member

mcarrano left a comment

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 ?

@nicolethoen

This comment has been minimized.

Copy link
Contributor Author

nicolethoen commented Oct 16, 2019

@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.

@mcarrano

This comment has been minimized.

Copy link
Member

mcarrano commented Oct 16, 2019

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).

Copy link
Member

dlabrecq left a comment

LGTM

Copy link
Member

mcarrano left a comment

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

@@ -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: 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 */

This comment has been minimized.

Copy link
@tlabaj

tlabaj Oct 17, 2019

Contributor

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

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

This comment has been minimized.

Copy link
@tlabaj

tlabaj Oct 17, 2019

Contributor

can you make this sentence case.

@nicolethoen nicolethoen dismissed stale reviews from mcarrano and dlabrecq via e1b1374 Oct 17, 2019
@nicolethoen nicolethoen force-pushed the nicolethoen:toolbar_changes branch 3 times, most recently from 046f4cd to 595e376 Oct 18, 2019
@nicolethoen

This comment has been minimized.

Copy link
Contributor Author

nicolethoen commented Oct 21, 2019

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

@mattnolting

This comment has been minimized.

Copy link
Contributor

mattnolting commented Oct 21, 2019

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

Copy link
Contributor

tlabaj left a comment

LGTM

Copy link
Member

mcarrano left a comment

LGTM!

Copy link
Contributor

mattnolting left a comment

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

Screen Shot 2019-10-21 at 4 17 57 PM

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

This comment has been minimized.

Copy link
@mattnolting

mattnolting Oct 21, 2019

Contributor
Suggested change
<Button variant="link" onClick={clearChipGroups}>
<Button variant="link" onClick={clearChipGroups} isInline>
Copy link
Contributor

mattnolting left a comment

Toolbar height shrinks when toggle is opened.

Screen Shot 2019-10-21 at 4 22 09 PM

Screen Shot 2019-10-21 at 4 22 15 PM

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

@nicolethoen nicolethoen dismissed stale reviews from dlabrecq and mturley via 040dd0b Oct 22, 2019
Copy link
Collaborator

mturley left a comment

🌯 🌯

@nicolethoen nicolethoen force-pushed the nicolethoen:toolbar_changes branch from 040dd0b to f4be16f Oct 22, 2019
Copy link
Collaborator

mturley left a comment

🌯 🌯 🌯

Copy link
Contributor

mattnolting left a comment

@nicolethoen nicolethoen dismissed stale reviews from mattnolting and mturley via db5dd59 Oct 23, 2019
@nicolethoen nicolethoen force-pushed the nicolethoen:toolbar_changes branch from f4be16f to db5dd59 Oct 23, 2019
Copy link
Contributor

mattnolting left a comment

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

@mturley mturley merged commit e77c613 into patternfly:master Oct 23, 2019
8 checks passed
8 checks passed
ci/circleci: build Your tests passed on CircleCI!
Details
ci/circleci: build_integration Your tests passed on CircleCI!
Details
ci/circleci: build_pf3_docs Your tests passed on CircleCI!
Details
ci/circleci: build_pf4_docs Your tests passed on CircleCI!
Details
ci/circleci: lint Your tests passed on CircleCI!
Details
ci/circleci: test_jest_other Your tests passed on CircleCI!
Details
ci/circleci: test_jest_pf4 Your tests passed on CircleCI!
Details
ci/circleci: upload_docs Your tests passed on CircleCI!
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.