Skip to content

feat(Drawer, DataList, Page): rename noPadding to hasNoPadding #4133

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.

Sign up for GitHub

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

Jump to bottom
Merged
redallen merged 2 commits into from
May 7, 2020
Merged

feat(Drawer, DataList, Page): rename noPadding to hasNoPadding #4133

redallen merged 2 commits into from
May 7, 2020

Conversation

@kmcfaul
Copy link
Contributor

@kmcfaul kmcfaul commented Apr 27, 2020 •
edited
Loading

And add Page padding breakpoint logic

What: Closes #4013, #4039, #4040

Breaking changes

  1. Renamed noPadding to hasNoPadding for Drawer, DataList and Page components.
  2. Added hasPaddingOn and hasNoPaddingOn properties to PageSection, accounting for page size breakpoints. Breakpoints are defined in the PageSectionBreakpoints enum.

@patternfly-build
Copy link
Collaborator

patternfly-build commented Apr 27, 2020 •
edited
Loading

PF4 preview: https://patternfly-react-pr-4133.surge.sh
A11y report: https://patternfly-react-pr-4133-coverage.surge.sh

@jenny-s51 jenny-s51 mentioned this pull request Apr 27, 2020
Merged
dlabrecq
dlabrecq approved these changes Apr 28, 2020
View reviewed changes
Copy link
Member

@dlabrecq dlabrecq left a comment •
edited
Loading

Choose a reason for hiding this comment

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

hasNoPadding sounds weird to me, but looks good otherwise. I might have gone with hasPadding and default true

jessiehuff
jessiehuff approved these changes Apr 28, 2020
View reviewed changes
Copy link
Contributor

@jessiehuff jessiehuff left a comment

Choose a reason for hiding this comment

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

LGTM!

boaz0
boaz0 approved these changes Apr 29, 2020
View reviewed changes
Copy link
Member

@boaz0 boaz0 left a comment

Choose a reason for hiding this comment

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

👍

@tlabaj tlabaj added the Breaking change 💥 this change requires a major release and has API changes. label Apr 29, 2020
tlabaj
tlabaj previously requested changes Apr 29, 2020
View reviewed changes
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. Can you please add the Breaking change comments to description.
Can you also update demo-app and integration test with the new props.

redallen
redallen suggested changes May 4, 2020
View reviewed changes
Copy link
Contributor

@redallen redallen left a comment

Choose a reason for hiding this comment

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

Also have a merge conflict. Thanks for working on this @kmcfaul !

packages/react-core/src/components/DataList/DataListContent.tsx
isHidden?: boolean;
/** Flag to remove padding from the expandable content */
noPadding?: boolean;
hasNoPadding?: boolean;
Copy link
Contributor

@redallen redallen May 4, 2020

Choose a reason for hiding this comment

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

Can we change this to hasPadding and default it to true? Sounds a little better and codemods fix it all the same.

Copy link
Contributor Author

@kmcfaul kmcfaul May 6, 2020

Choose a reason for hiding this comment

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

@tlabaj Thoughts?

I don't mind either way.

Copy link
Contributor Author

@kmcfaul kmcfaul May 6, 2020

Choose a reason for hiding this comment

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

Keeping as hasNoPadding per standup discussion for now

@kmcfaul kmcfaul force-pushed the breaking-paddingvar branch from 39ddcf2 to 983d190 Compare May 6, 2020 17:22
@kmcfaul kmcfaul force-pushed the breaking-paddingvar branch from 983d190 to f75f6e1 Compare May 6, 2020 18:24
redallen
redallen approved these changes May 7, 2020
View reviewed changes
Copy link
Contributor

@redallen redallen left a comment

Choose a reason for hiding this comment

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

I think hasNo props should just be has, but this is outside the scope of this PR. LGTM!

@redallen redallen dismissed tlabaj’s stale review May 7, 2020 16:13

Comments added and tests updated.

@redallen redallen merged commit 9d7ee63 into patternfly:v4 May 7, 2020
This was referenced May 7, 2020
Closed
Closed
@patternfly-build
Copy link
Collaborator

patternfly-build commented May 7, 2020

Your changes have been released in:

  • @patternfly/react-catalog-view-extension@4.2.22
  • @patternfly/react-core@4.10.0
  • @patternfly/react-docs@5.2.26
  • @patternfly/react-inline-edit-extension@4.3.22
  • demo-app-ts@4.7.0
  • @patternfly/react-integration@4.7.0
  • @patternfly/react-table@4.3.22
  • @patternfly/react-topology@4.2.22
  • @patternfly/react-virtualized-extension@4.2.22

Thanks for your contribution! 🎉

@redallen redallen mentioned this pull request May 19, 2020
Closed
1 task
@nicolethoen nicolethoen mentioned this pull request Jul 19, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dlabrecq dlabrecq dlabrecq approved these changes

@tlabaj tlabaj tlabaj left review comments

+3 more reviewers

@boaz0 boaz0 boaz0 approved these changes

@jessiehuff jessiehuff jessiehuff approved these changes

@redallen redallen redallen approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Breaking change 💥 this change requires a major release and has API changes.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Page - updates to noPadding*

7 participants

@kmcfaul @patternfly-build @boaz0 @dlabrecq @jessiehuff @tlabaj @redallen