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

feat(DataToolbar): Add Data Toolbar component to experimental #2861

Merged
merged 22 commits into from Sep 10, 2019

Conversation

@nicolethoen
Copy link
Contributor

commented Sep 6, 2019

@patternfly-build

This comment has been minimized.

Copy link
Contributor

commented Sep 6, 2019

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

@karelhala
Copy link
Contributor

left a comment

Thank you so much for this PR! It's a great start, we just have to remove the isExpanded from state and some variable renames and we are good to go!

}

componentDidMount() {
window.addEventListener('resize', this.closeExpandableContent);

This comment has been minimized.

Copy link
@karelhala

karelhala Sep 6, 2019

Contributor

Please do not listen on resize events and close expandable content. This is css breakpoints work, no need to close the content on each window resize. It adds performance issues and plus unwanted behavior.

This comment has been minimized.

Copy link
@dlabaj

dlabaj Sep 6, 2019

Contributor

@karelhala There's an issue where if the toggle drawer is open and the window is resized when the breakpoint is hit the content will not layout correctly. We're looking for a solution to make it behave the same way that dropdown does. Will look at that code to see how that one works to see if there's a better solution.

This comment has been minimized.

Copy link
@karelhala

karelhala Sep 6, 2019

Contributor

This is really unaesthetic because imagine that you are looking at table with this on mobile, click on the expand button, realise that landscape view might be better for this and once you flip the phone this expanded menu is lost and user has to click on it once again.

If the layout is not rendered correctly on window resize I think that is more of a issue in PF core and we shouldn't try to fix it here.

This comment has been minimized.

Copy link
@dlabaj

dlabaj Sep 6, 2019

Contributor

@mcarrano Any thoughts on the behavior with the toggle?

This comment has been minimized.

Copy link
@mcarrano

mcarrano Sep 6, 2019

Member

This is a tough one to answer because it's not clear what the best approach is. I agree that just collapsing the drawer on resize it not great. Maybe if the drawer is open and a breakpoint is crossed, the content should not reconfigure itself (i.e. suppress the behavior in this case). Is that even possible? @mattnolting any thoughts here?

This comment has been minimized.

Copy link
@nicolethoen

nicolethoen Sep 6, 2019

Author Contributor

@mcarrano - matt nolting is on PTO until Tuesday.

regarding that being possible, yes I believe it is, but we'd have to use the breakpoint passed in by the user and look up the global size of that breakpoint and make it reconfigure itself based on the global size (hopefully no one will have over written it). I may not have time to figure that out today - but I can try. Would that be an acceptable approach @dlabaj?

This comment has been minimized.

Copy link
@mattnolting

mattnolting Sep 10, 2019

@mcarrano I think, when using a toggle group, the content should reconfigure at the specified breakpoint if expanded content is .pf-m-expanded.

@mattnolting
Copy link

left a comment

Well done @nicolethoen!!! It's really exciting to see all the great work you've done in action. Left some comments.

@mturley
Copy link
Collaborator

left a comment

Awesome work @nicolethoen !

{toggleIcon}
</Button>
</div>
{ isExpanded ? ReactDOM.createPortal(children, expandableContentRef.current): children }

This comment has been minimized.

Copy link
@mturley

mturley Sep 9, 2019

Collaborator

I'm so glad this worked! For a while there I thought there might not be a declarative way to achieve this... but context, refs and portals = magic

@nicolethoen nicolethoen changed the title [WIP] feat(DataToolbar): Add Data Toolbar component to experimental feat(DataToolbar): Add Data Toolbar component to experimental Sep 9, 2019

@nicolethoen nicolethoen force-pushed the nicolethoen:toolbar_component branch from 97e8bed to b112b4f Sep 9, 2019

@mturley
Copy link
Collaborator

left a comment

Noticed a few non-blocking things, but this looks good to me.

</Select>
</DataToolbarItem>
<DataToolbarItem>
<Select

This comment has been minimized.

Copy link
@mturley

mturley Sep 10, 2019

Collaborator

There's some inconsistent indentation/spacing here and elsewhere in this file, but I don't think we need to hold up the PR for it. I'll address cases like this when I rebase #2810 after the release, but I'm not sure if Prettier will run on the JSX in our Markdown files, so that's something else I want to look into.

variant="plain"
onClick={toggleIsExpanded}
{...isExpanded && { 'aria-expanded': true }}
// TODO aria-haspopup when isExpanded = true && viewport is smaller than lg global breakpoint

This comment has been minimized.

Copy link
@mturley

mturley Sep 10, 2019

Collaborator

Is this something we need to implement with this PR, and if not can we open an issue to track it?

This comment has been minimized.

Copy link
@nicolethoen

nicolethoen Sep 10, 2019

Author Contributor

good thought

This comment has been minimized.

Copy link
@dlabaj

dlabaj Sep 10, 2019

Contributor

Let's open an issue for this.

This comment has been minimized.

Copy link
@nicolethoen

nicolethoen Sep 10, 2019

Author Contributor

Issue created

@mattnolting
Copy link

left a comment

@nicolethoen Again, great work! Can we add an example of .pf-m-space-items on a group to the Data toolbar spacers example? Perhaps the last two action buttons?

@nicolethoen can we add a horizontal divider to the Data toolbar group stacked example?

@mcarrano @mceledonia should we add a horizontal divider to the Data toolbar group stacked example?

@nicolethoen

This comment has been minimized.

Copy link
Contributor Author

commented Sep 10, 2019

@nicolethoen Again, great work! Can we add an example of .pf-m-space-items on a group to the Data toolbar spacers example? Perhaps the last two action buttons?

@nicolethoen can we add a horizontal divider to the Data toolbar group stacked example?

Yes and Yes!

@mcarrano

This comment has been minimized.

Copy link
Member

commented Sep 10, 2019

@nicolethoen This is looking great. Agree with adding the horizontal divider to the stacked example. The only other thing I found is that for the Consumer managed toggle group example, I can get into a situation where the drawer remains open but the toggle disappears by opening the drawer and then stretching the window. See below.

Screen Shot 2019-09-10 at 11 24 30 AM

If this is not easy to fix. I'd be OK to approve this and come back to address this as a bug in the next release.

@karelhala

This comment has been minimized.

Copy link
Contributor

commented Sep 10, 2019

@mcarrano that's exactly what is discussed in #2861 (comment)

Consumers should be rensponsible when the pane is hidden and shown in this example. So if you open this toggle change the screen size and want to close the pane consumer is responsible for that. It's actually expected bahavior.

@nicolethoen

This comment has been minimized.

Copy link
Contributor Author

commented Sep 10, 2019

@nicolethoen This is looking great. Agree with adding the horizontal divider to the stacked example. The only other thing I found is that for the Consumer managed toggle group example, I can get into a situation where the drawer remains open but the toggle disappears by opening the drawer and then stretching the window. See below.

Screen Shot 2019-09-10 at 11 24 30 AM

If this is not easy to fix. I'd be OK to approve this and come back to address this as a bug in the next release.

I noted this issue in the documentation above the example.

@dlabaj
dlabaj approved these changes Sep 10, 2019
Copy link
Contributor

left a comment

A few comments but looks good otherwise.

className?: string;
/** Content to be rendered as rows in the Data toolbar */
children?: React.ReactNode;
/** Flag indicating if a Data toolbar toggle group's expandable content is expanded */

This comment has been minimized.

Copy link
@dlabaj

dlabaj Sep 10, 2019

Contributor

We should add a comment that setting this means that the consumer will be responsible for managing the toogle group expansion. Also how would a consumer find out if the toogle group is currently expanded after construction??? Can we use a getter to return the correct value.

This comment has been minimized.

Copy link
@mturley

mturley Sep 10, 2019

Collaborator

If the consumer is managing the expansion, they would already know, right? If the consumer is not managing the expansion, why do they need to be able to find out if it's currently expanded?

This comment has been minimized.

Copy link
@nicolethoen

nicolethoen Sep 10, 2019

Author Contributor

There is some documentation just before the toggle group examples that i believe explains this? should I add more info to that documentation?

variant="plain"
onClick={toggleIsExpanded}
{...isExpanded && { 'aria-expanded': true }}
// TODO aria-haspopup when isExpanded = true && viewport is smaller than lg global breakpoint

This comment has been minimized.

Copy link
@dlabaj

dlabaj Sep 10, 2019

Contributor

Let's open an issue for this.

@mcarrano
Copy link
Member

left a comment

Looks great @nicolethoen !

@mcarrano mcarrano added ux approved and removed ux review labels Sep 10, 2019

@mattnolting
Copy link

left a comment

Just two more comments 😄

<DataToolbarItem spacers={fifthSpacers}><Button variant="secondary">Action</Button></DataToolbarItem>
<DataToolbarItem><Button variant="primary">Action</Button></DataToolbarItem>
<DataToolbarItem variant="separator"></DataToolbarItem>
<DataToolbarGroup itemSpacers={[{spacerSize: 'lg'}]} spacers={[{spacerSize: 'lg'}]}>

This comment has been minimized.

Copy link
@mattnolting

mattnolting Sep 10, 2019

Can you remove spacers={[{spacerSize: 'lg'}]}

<DataToolbarItem variant="separator"></DataToolbarItem>
<DataToolbarGroup itemSpacers={[{spacerSize: 'lg'}]} spacers={[{spacerSize: 'lg'}]}>
<DataToolbarItem><Button variant="secondary">Action</Button></DataToolbarItem>
` <DataToolbarItem><Button variant="secondary">Action</Button></DataToolbarItem>

This comment has been minimized.

Copy link
@mattnolting

mattnolting Sep 10, 2019

looks like you have a floating tick mark ^^

@nicolethoen nicolethoen dismissed stale reviews from mcarrano and dlabaj via abe78d6 Sep 10, 2019

@mattnolting
Copy link

left a comment

I love it!! 🎉 LGTM

@tlabaj
tlabaj approved these changes Sep 10, 2019
Copy link
Contributor

left a comment

LGTM

@mturley
Copy link
Collaborator

left a comment

LGTM!

@dlabaj
dlabaj approved these changes Sep 10, 2019

@dlabaj dlabaj merged commit 6ec0ce4 into patternfly:master Sep 10, 2019

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

This comment has been minimized.

Copy link
Contributor

commented Sep 10, 2019

Your changes have been released in:

  • @patternfly/react-core@3.104.0
  • @patternfly/react-docs@4.13.0
  • @patternfly/react-inline-edit-extension@2.11.35
  • demo-app-ts@2.24.8
  • @patternfly/react-table@2.20.15
  • @patternfly/react-topology@2.8.34
  • @patternfly/react-virtualized-extension@1.2.23

Thanks for your contribution! 🎉

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.