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

feat(react-core/card): added PatternFly 4 Card Component #458

Merged
merged 1 commit into from
Jul 26, 2018
Merged

feat(react-core/card): added PatternFly 4 Card Component #458

merged 1 commit into from
Jul 26, 2018

Conversation

dmiller9911
Copy link
Contributor

affects: @patternfly/react-core, @patternfly/react-docs

Added the Card Component from here: https://pf-next.com/components/Card/examples/

ISSUES CLOSED: #386

@dmiller9911 dmiller9911 added the PF4 label Jul 5, 2018
@coveralls
Copy link

Pull Request Test Coverage Report for Build 1687

  • 24 of 24 (100.0%) changed or added relevant lines in 4 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.2%) to 74.682%
Totals Coverage Status
Change from base Build 1686: 0.2%
Covered Lines: 2135
Relevant Lines: 2633
💛 - Coveralls

@patternfly-build
Copy link
Contributor

patternfly-build commented Jul 5, 2018 •
edited
Loading

Deploy preview for patternfly-react ready!

Built with commit 860aeca

https://deploy-preview-458--patternfly-react.netlify.com

karelhala
karelhala previously approved these changes Jul 13, 2018
View reviewed changes
Copy link
Contributor

@karelhala karelhala left a comment

Choose a reason for hiding this comment

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

LGTM

@priley86
Copy link
Member

no issues here, just need to rebase the commit and remove the /...will be happy to approve/merge after that...

@jgiardino
Copy link
Collaborator

The only potential concern I have is about the default html elements defined. I realize that a request was made in PF4 core to default cards to <article>. For this element, using <header> and <footer> makes sense. But if another element is defined that is either not a sectioning content element or sectioning root element, then <header> should not be used, as stated by the w3c:

The <header> element represents introductory content for its nearest ancestor <main> element, sectioning content, or sectioning root element.

Does anyone have thoughts or suggestions on that? I'm torn between using these defaults, which are likely to be the right elements to use for most cases, versus requiring consumers to explicitly set what they want these elements to be. My concern is that if we make assumptions and set defaults, and the consumer isn't fully aware, then we're partly responsible for bad semantics. Is it possible to switch the default from <header> to <div> if the parent element is not a sectioning content or sectioning root element?

@jgiardino
Copy link
Collaborator

This comment may be out of scope for this PR, but when I look at the preview, it looks like I'm only seeing details about the parent Card component, and not the children components being used, like CardBody and CardHeader. Would you expect all components that are used to create the example to be represented in the Props section?

@dmiller9911
Copy link
Contributor Author

@jgiardino What is the risk of header/footer being used outside of a sectioning element? I can't find that in the docs it just mentions to not do it. Is it treated like a div? Also what is going to be the majority use on these? I am cool to use divs as defaults, but if users are meant to make them articles 90% of the time I think it makes sense to default to article. As for verifying there is not a good way to do this in all honesty. Anything we would do would be hacky, hard to maintain, and not very performant. For the props table that is a known issue. I plan to fix that soon.

@dmiller9911 dmiller9911 force-pushed the react-core/cards branch 2 times, most recently from 5df1cfb to 82325a0 Compare July 17, 2018 17:28
@jgiardino
Copy link
Collaborator

What is the risk of header/footer being used outside of a sectioning element?

@dmiller9911 My best guess is that the first occurrence of <header> will be considered the header for the <main> section if <main> is present as an ancestor to the Card, or <body> if <main> isn't present.

I'm on the fence for this one. I'm guessing this situation could come up again for other components. I added this as an agenda topic for our weekly a11y review on Thursdays. I'm also copying @andresgalante on this one in case he has any suggestions before then.

While I'm all for promoting the use of semantically correct elements, I don't like making assumptions about the consumer's use case. Something seems off about requiring the consumer to 1.) understand that default elements are defined, and 2.) explicitly define generic elements if the default ones aren't appropriate for their use case.

One possible solution is to use a default <article> element for the parent <Card> component, but require the consumer to explicitly set <header> as the element for <CardHeader> and <footer> for the <CardFooter>.

@dmiller9911
Copy link
Contributor Author

@jgiardino That seems like a good middle ground.

@dmiller9911
feat(react-core-card): added PatternFly 4 Card Component
860aeca 
affects: @patternfly/react-core, @patternfly/react-docs

Added the Card Component from here: https://pf-next.com/components/Card/examples/

ISSUES CLOSED: patternfly#386
@dmiller9911
Copy link
Contributor Author

@jgiardino mind taking a look and giving the thumbs up for this? The extra component docs will be coming soon with a different PR. Most likely early next week. Right now Card defaults to article. Header and Footer default to divs.

karelhala
karelhala approved these changes Jul 26, 2018
View reviewed changes
Copy link
Contributor

@karelhala karelhala left a comment

Choose a reason for hiding this comment

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

LGTM

jgiardino
jgiardino approved these changes Jul 26, 2018
View reviewed changes
Copy link
Collaborator

@jgiardino jgiardino left a comment

Choose a reason for hiding this comment

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

Looks great!

@dmiller9911 dmiller9911 merged commit a9af02d into patternfly:master Jul 26, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jgiardino jgiardino jgiardino approved these changes

@karelhala karelhala karelhala approved these changes

@dgutride dgutride Awaiting requested review from dgutride

@jeff-phillips-18 jeff-phillips-18 Awaiting requested review from jeff-phillips-18

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@dmiller9911 @coveralls @patternfly-build @priley86 @jgiardino @karelhala