-
Notifications
You must be signed in to change notification settings - Fork 5
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.
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
USWDS-Next - Alpha: Create usa-card
component
#13
base: develop
Are you sure you want to change the base?
Conversation
usa-link
componentusa-card
component
usa-card
componentusa-card
component
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this! Couple of suggestions in there about supporting theming better and adding some documentation, but a really solid start!
@@ -0,0 +1,24 @@ | |||
import { LitElement, html, unsafeCSS } from "lit"; | |||
import usaCardStyle from "@uswds/uswds/scss/usa-card?inline"; | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: Could you please add some JSDoc comments to explain the props/attributes for this component? If you end up adding any style hooks, add those in too. Take a look at the supported metadata for custom element manifests to see what's available.
box-sizing: border-box; | ||
} | ||
|
||
[slot="card-heading"] { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: Consider using a part here for styling since this seems like a good place to support theming.
@@ -0,0 +1,23 @@ | |||
import uswdsCoreStyle from "@uswds/uswds/scss/uswds-core?inline"; | |||
import usaCardStyle from "@uswds/uswds/scss/usa-card?inline"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: Consider adding some CSS variable versions of design tokens to support theming here.
@@ -0,0 +1,23 @@ | |||
import uswdsCoreStyle from "@uswds/uswds/scss/uswds-core?inline"; | |||
import usaCardStyle from "@uswds/uswds/scss/usa-card?inline"; | |||
import usaButtonStyle from "@uswds/uswds/scss/usa-button?inline"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: Same suggestion re: CSS variables for card theming.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for starting this. Added comments to better follow lit standards and improve clarity.
What I tested
- Code passes
npm run prettier:js
- Creating custom card elements without errors
- Card element follows lit standards and best practices
import "./index"; | ||
import "../usa-card-group/index"; | ||
|
||
import { html, nothing} from "lit"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
polish: Can we fix the spacing in braces?
args: { | ||
title: "", | ||
media: "https://designsystem.digital.gov/img/introducing-uswds-2-0/built-to-grow--alt.jpg", | ||
content: "Lorem ipsum dolor sit amet consectetur adipisicing elit. Facilis earum tenetur quo cupiditate, eaque qui officia recusandae.", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question: Does this mean I can't pass in custom markup to the card body?
title: "", | ||
media: "https://designsystem.digital.gov/img/introducing-uswds-2-0/built-to-grow--alt.jpg", | ||
content: "Lorem ipsum dolor sit amet consectetur adipisicing elit. Facilis earum tenetur quo cupiditate, eaque qui officia recusandae.", | ||
buttonText: "Visit Florida Keys", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue (non-blocking): Currently there's no way for users to pass their own CTA.
For example, if I wanted to pass a button instead of anchor. Or if I wanted to set a custom href
.
} | ||
} | ||
|
||
window.customElements.define("usa-card", UsaCard); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
window.customElements.define("usa-card", UsaCard); | |
customElements.define("usa-card", UsaCard); |
} | ||
} | ||
|
||
window.customElements.define("usa-card-group", UsaCardGroup); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
window.customElements.define("usa-card-group", UsaCardGroup); | |
customElements.define("usa-card-group", UsaCardGroup); |
cardTemplate() { | ||
const classes = { | ||
"usa-card": true, | ||
"usa-card--header-first": this.headerFirst, | ||
"usa-card--flag": this.layout === "flag" || this.layout === "flag-alt", | ||
"usa-card--media-right": this.layout == "flag-alt", | ||
} | ||
return html` | ||
<div class="${classMap(classes)}"> | ||
<div class="usa-card__container"> | ||
${this.headerTemplate()} | ||
${this.mediaTemplate()} | ||
${this.bodyTemplate()} | ||
${this.footerTemplate()} | ||
</div> | ||
</div> | ||
` | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue: This function seems redundant and could instead be moved to the render()
.
} | ||
|
||
const classes = { | ||
"usa-card__media": true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue: Let's move static classes, like usa-card__media
to the element and reserve class maps for dynamic.
Example
const classes = {
- "usa-card__media": true,
}
+ <div class="usa-card__media ${classMap(classes)}">
"usa-card__media--inset": this.media.hasAttribute("inset") && !this.media.hasAttribute("exdent"), | ||
"usa-card__media--exdent": this.exdent || this.media.hasAttribute("exdent") && !this.media.hasAttribute("indent"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thought: Just an idea. A new attribute placement="extend | indent | none"
might be useful here.
Possible alternative names: padding, position, spacing.
this.headerContent = [...this.header.children]; | ||
this.media = this.querySelector("[slot='card-media']") | ||
this.body = this.querySelector("[slot='card-body']"); | ||
this.bodyContent = [...this.body.children]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue: If not defined I get an error.
Error
VM784 index.js:26 Uncaught TypeError: Cannot read properties of null (reading 'children')
at new UsaCard (VM784 index.js:26:38)
at TemplateInstance._clone (VM740 chunk-MPCHNED3.js:354:80)
at _ChildPart._commitTemplateResult (VM740 chunk-MPCHNED3.js:590:33)
at _ChildPart._$setValue (VM740 chunk-MPCHNED3.js:486:12)
at render (VM740 chunk-MPCHNED3.js:934:8)
at renderToCanvas (VM728 @storybook_web-components_dist_entry-preview__mjs.js:48:5)
at StoryRender.renderToScreen (VM713 runtime.js:147:9014)
at VM713 runtime.js:144:3305
at StoryRender.runPhase (VM713 runtime.js:144:943)
at StoryRender.render (VM713 runtime.js:144:3238)
Sample markup
<usa-card>
<div slot="card-header">
<h1>My title</h1>
</div>
Hello
</usa-card>
this.footer = this.querySelector("[slot='card-footer']"); | ||
this.footerContent = [...this.footer.children]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue: If not defined I get an error.
Error
index.js:28 Uncaught TypeError: Cannot read properties of null (reading 'children')
at new UsaCard (index.js:28:42)
at TemplateInstance._clone (chunk-MPCHNED3.js?v=1f2e655f:354:80)
at _ChildPart._commitTemplateResult (chunk-MPCHNED3.js?v=1f2e655f:590:33)
at _ChildPart._$setValue (chunk-MPCHNED3.js?v=1f2e655f:486:12)
at render (chunk-MPCHNED3.js?v=1f2e655f:934:8)
at sourceDecorator (@storybook_web-components_dist_entry-preview-docs__mjs.js?v=1f2e655f:162:108)
at hookified (runtime.js:97:10605)
at runtime.js:113:3361
at runtime.js:113:4023
at runtime.js:97:11408
Sample markup
<usa-card>
<div slot="card-body">
<h1>My title</h1>
Hello from card body.
</div>
</usa-card>
Summary
Created an alpha version of a
usa-card
web componentRelated issue
Closes #16
Requirements notes
Variants
Accessibility
See related accessibility tests
Attributes & Props
header-first
<usa-card>
inset
<img>
exdent
<usa-card>
exdent
styles to the header, body, and footer divsexdent
styles to specific card elementslayout
<usa-card>