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.

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

docs(Card): add accordion, trend demos #6451

Merged
merged 4 commits into from
Oct 19, 2021
Merged

docs(Card): add accordion, trend demos #6451

merged 4 commits into from
Oct 19, 2021

Conversation

kmcfaul
Copy link
Contributor

@kmcfaul kmcfaul commented Oct 14, 2021

What: Closes #6411, #6412, #6413

Adds the remaining missing card demos.

@patternfly-build
Copy link
Contributor

patternfly-build commented Oct 14, 2021 •
edited
Loading

PF4 preview: https://patternfly-react-pr-6451.surge.sh

nicolethoen
nicolethoen previously approved these changes Oct 14, 2021
View reviewed changes
Copy link
Contributor

@nicolethoen nicolethoen left a comment

Choose a reason for hiding this comment

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

LGTM!
The charts in the accordion demo don't have a light grey border like the charts in the core demo do. I imagine that's because the core demos are using images and you are using 'live' charts.

tlabaj
tlabaj previously approved these changes Oct 18, 2021
View reviewed changes
mcoker
mcoker reviewed Oct 18, 2021
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.

Just one comment. Do you think the accordion is better as a definition list than with sub headings?

packages/react-core/src/demos/CardDemos.md Outdated Show resolved Hide resolved
@kmcfaul kmcfaul dismissed stale reviews from tlabaj and nicolethoen via 4e8d08d October 18, 2021 18:36
mcoker
mcoker approved these changes Oct 18, 2021
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.

L🥳TM!

mcarrano
mcarrano reviewed Oct 18, 2021
View reviewed changes
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.

Just one question... It might not matter, but why is the spacing/scaling slightly different between the core and React versions of the Trend cards.

React

Screen Shot 2021-10-18 at 4 25 32 PM

Core HTML

Screen Shot 2021-10-18 at 4 25 46 PM

@kmcfaul
Copy link
Contributor Author

kmcfaul commented Oct 18, 2021

If you mean the spacing around the chart, it's probably due to using the actual Chart versus an image.

@mcoker
Copy link
Contributor

mcoker commented Oct 18, 2021

@mcarrano the dropdown toggle icon is more narrow in core (10px font awesome) than in react (16px SVG). And the chart in core is just a screenshot, the react demo includes an actual chart.

mcarrano
mcarrano approved these changes Oct 18, 2021
View reviewed changes
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.

Thanks @mcoker . I'm good then!

@tlabaj tlabaj merged commit 65a87f5 into patternfly:main Oct 19, 2021
This was referenced Oct 19, 2021
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mcarrano mcarrano mcarrano approved these changes

@nicolethoen nicolethoen nicolethoen approved these changes

@mcoker mcoker mcoker approved these changes

@tlabaj tlabaj tlabaj approved these changes

Assignees
No one assigned
Labels
css approved ux approved
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

With accordion
6 participants
@kmcfaul @patternfly-build @mcoker @mcarrano @nicolethoen @tlabaj