Skip to content

feat(Card): Refactor card header #8759

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
kmcfaul merged 7 commits into from
Mar 9, 2023
Merged

feat(Card): Refactor card header #8759

kmcfaul merged 7 commits into from
Mar 9, 2023

Conversation

@tlabaj
Copy link
Contributor

@tlabaj tlabaj commented Feb 28, 2023 •
edited
Loading

What: Closes #8684

  • Update the card header API.
  • Removed CardHead main component. IT was not being used.
  • some clean up
  • removed the example file CardOnlyImageInCardHead.tsx since it was not bing used

Updated @patternfly/documentation-framework to the latest alpha version to fix build problem by pulling in patternfly/patternfly-org#3429

@patternfly-build
Copy link
Collaborator

patternfly-build commented Feb 28, 2023 •
edited
Loading

Preview: https://patternfly-react-pr-8759.surge.sh

A11y report: https://patternfly-react-pr-8759-a11y.surge.sh

@mcoker mcoker requested a review from srambach February 28, 2023 21:29
This was referenced Feb 28, 2023
Closed
Closed
@srambach
Copy link
Member

srambach commented Mar 1, 2023

If I'm reading it right, it looks like this is missing the .pf-c-card__header-main that is now required.
Also, the images seem to be missing from the cards, but maybe that's a surge issue?

image

thatblindgeye
thatblindgeye requested changes Mar 1, 2023
View reviewed changes
@tlabaj
Copy link
Contributor Author

tlabaj commented Mar 1, 2023

If I'm reading it right, it looks like this is missing the .pf-c-card__header-main that is now required. Also, the images seem to be missing from the cards, but maybe that's a surge issue?

@srambach Good catch. You are correct. The CardHeaderMain componentwas missing a class. The class is also missing inmain` (v4) branch. I could open an issue and resolve it for the special release of v4. What do you think?

@srambach
Copy link
Member

srambach commented Mar 1, 2023

If I'm reading it right, it looks like this is missing the .pf-c-card__header-main that is now required. Also, the images seem to be missing from the cards, but maybe that's a surge issue?

@srambach Good catch. You are correct. The CardHeaderMain componentwas missing a class. The class is also missing inmain` (v4) branch. I could open an issue and resolve it for the special release of v4. What do you think?

.pf-c-card__header-main wasn't required in v4 - only if there was an icon or image that needed to be aligned, but I don't think it's harmful to be there either.

@tlabaj
Copy link
Contributor Author

tlabaj commented Mar 1, 2023 •
edited
Loading

If I'm reading it right, it looks like this is missing the .pf-c-card__header-main that is now required. Also, the images seem to be missing from the cards, but maybe that's a surge issue?

@srambach Good catch. You are correct. The CardHeaderMain componentwas missing a class. The class is also missing inmain` (v4) branch. I could open an issue and resolve it for the special release of v4. What do you think?

.pf-c-card__header-main wasn't required in v4 - only if there was an icon or image that needed to be aligned, but I don't think it's harmful to be there either.

@srambach What I mean is that the CardHederMain div is missing the class in v4.

image

thatblindgeye
thatblindgeye approved these changes Mar 3, 2023
View reviewed changes
Copy link
Contributor

@thatblindgeye thatblindgeye left a comment

Choose a reason for hiding this comment

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

Just needs merge conflicts resolved

mcoker
mcoker approved these changes Mar 3, 2023
View reviewed changes
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.

LGTM just one small docs thing, not a blocker.

kmcfaul
kmcfaul approved these changes Mar 9, 2023
View reviewed changes
Copy link
Contributor

@kmcfaul kmcfaul 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 to me!

@tlabaj @mcoker
Update packages/react-core/src/components/Card/CardHeader.tsx
101de2b 
Co-authored-by: Michael Coker <35148959+mcoker@users.noreply.github.com>
@kmcfaul kmcfaul merged commit 387da19 into patternfly:v5 Mar 9, 2023
@patternfly-build
Copy link
Collaborator

patternfly-build commented Mar 9, 2023

Your changes have been released in:

  • @patternfly/react-code-editor@5.0.0-alpha.29
  • @patternfly/react-core@5.0.0-alpha.29
  • @patternfly/react-docs@6.0.0-alpha.32
  • demo-app-ts@5.0.0-alpha.12
  • @patternfly/react-integration@5.0.0-alpha.6
  • @patternfly/react-table@5.0.0-alpha.29

Thanks for your contribution! 🎉

@thatblindgeye thatblindgeye mentioned this pull request Mar 13, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mcoker mcoker mcoker approved these changes

@kmcfaul kmcfaul kmcfaul approved these changes

@thatblindgeye thatblindgeye thatblindgeye approved these changes

@srambach srambach Awaiting requested review from srambach

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Card - Some classes are exposed to the consumer that should be rendered internally.

6 participants

@tlabaj @patternfly-build @srambach @mcoker @kmcfaul @thatblindgeye