Skip to content

Conversation

@kmcfaul
Copy link
Contributor

@kmcfaul kmcfaul commented May 15, 2023

What: Closes #8397

Adds gap, rowGap, and columnGap to Flex with breakpoints.
Adds examples & updates tests.

@mcoker The issue mentions a plain pf-m-gap class (without the size or breakpoints) that I don't see on the modifiers list, so I left it out for now. Did I miss something?

@tlabaj tlabaj requested review from mcoker and srambach May 15, 2023 20:37
@kmcfaul
Copy link
Contributor Author

kmcfaul commented May 15, 2023

Not sure why a Datepicker snapshot is failing here, should I update it?

Edit: rebase should fix it

@patternfly-build
Copy link
Contributor

patternfly-build commented May 15, 2023

Copy link
Contributor

@tlabaj tlabaj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good. Just a couple of nits.

I noticed that core has some text about spacing should we also have that here? Or should there be a follow up content issue @mcarrano @edonehoo ?

@tlabaj tlabaj requested a review from mcarrano May 16, 2023 14:31
@tlabaj tlabaj assigned tlabaj and unassigned tlabaj May 16, 2023
Copy link
Member

@mcarrano mcarrano left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tlabaj this looks good as far as I can tell. Regarding your comment about documentation, not sure what the core docs say, but agree that having some explanatory text for these examples would be helpful. If that can't be added quickly here, I'm OK to open a follow-up issue.

@tlabaj
Copy link
Contributor

tlabaj commented May 16, 2023

@tlabaj this looks good as far as I can tell. Regarding your comment about documentation, not sure what the core docs say, but agree that having some explanatory text for these examples would be helpful. If that can't be added quickly here, I'm OK to open a follow-up issue.

@mcarrano here is a link to the core content I was referring to https://pf5.patternfly.org/layouts/flex#spacing.

@mcarrano
Copy link
Member

I agree then that we should duplicate that documentation on the React page.

Copy link
Contributor

@mcoker mcoker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's beautiful! I created this core PR to enable the gap spacers without a size suffix (but can still be used with breakpoint suffix), not sure if we want to update this PR with that change or handle that as a follow-up after the core PR merges - patternfly/patternfly#5578

Copy link
Contributor

@tlabaj tlabaj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good. I am going to open up a follow up issue for content. I think the docs could be better organized and align more with Core docs.

@tlabaj
Copy link
Contributor

tlabaj commented May 17, 2023

it's beautiful! I created this core PR to enable the gap spacers without a size suffix (but can still be used with breakpoint suffix), not sure if we want to update this PR with that change or handle that as a follow-up after the core PR merges - patternfly/patternfly#5578

We can open follow up.

@tlabaj tlabaj merged commit 65f2b8b into patternfly:v5 May 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Flex layout - add support for gap spacing

6 participants