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.

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

Open
wants to merge 39 commits into
base: develop
Choose a base branch
from

Conversation

mahoneycm
Copy link
Contributor

@mahoneycm mahoneycm commented Jun 4, 2024

Summary

Created an alpha version of a usa-card web component

Related issue

Closes #16

Requirements notes

Variants

Name Description
Default Card component with a heading, text content, and a button
Card w/ Media A card variant featuring media.
Media with header first A card variant featuring media, displaying the header first.
Inset Media Indents the media element so it doesn’t extend to the edge of the card.
Exdent Media Extends the media element out over the card border.
Flag Display in a horizontal (“flag”) orientation at a specified width ($theme-card-flag-min-width).
Flag Media Right Inset Display in a horizontal (“flag”) orientation with media inset and on the right side.

Accessibility

See related accessibility tests

Attributes & Props

Attribute Element Description
header-first <usa-card> Enables header first variant when added to custom element tag
inset <img> Indents the media element so it doesn’t extend to the edge of the card.
exdent <usa-card> When added to the custom element, it applies exdent styles to the header, body, and footer divs
Component slots Can be added to individual card slots to add exdent styles to specific card elements
layout <usa-card> Can take `flag

@mahoneycm mahoneycm changed the title Add usa-link component [Playground] - Add usa-card component Jun 5, 2024
@mahoneycm mahoneycm marked this pull request as draft June 7, 2024 18:14
@mahoneycm mahoneycm mentioned this pull request Jun 11, 2024
Base automatically changed from jm-lit-banner to mh/lit-banner June 18, 2024 14:09
@mahoneycm mahoneycm changed the title [Playground] - Add usa-card component USWDS-Next - Alpha: Create usa-card component Jul 3, 2024
@mahoneycm mahoneycm changed the base branch from mh/lit-banner to develop July 3, 2024 19:43
@mahoneycm mahoneycm marked this pull request as ready for review July 3, 2024 20:41
Copy link
Collaborator

@heymatthenry heymatthenry left a 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";

Copy link
Collaborator

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"] {
Copy link
Collaborator

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";
Copy link
Collaborator

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";
Copy link
Collaborator

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.

Copy link
Collaborator

@mejiaj mejiaj left a 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";
Copy link
Collaborator

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.",
Copy link
Collaborator

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",
Copy link
Collaborator

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);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
window.customElements.define("usa-card", UsaCard);
customElements.define("usa-card", UsaCard);

}
}

window.customElements.define("usa-card-group", UsaCardGroup);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
window.customElements.define("usa-card-group", UsaCardGroup);
customElements.define("usa-card-group", UsaCardGroup);

Comment on lines +91 to +108
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>
`
}
Copy link
Collaborator

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,
Copy link
Collaborator

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)}">

Comment on lines +57 to +58
"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"),
Copy link
Collaborator

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];
Copy link
Collaborator

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>

Comment on lines +27 to +28
this.footer = this.querySelector("[slot='card-footer']");
this.footerContent = [...this.footer.children];
Copy link
Collaborator

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>

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants