-
Notifications
You must be signed in to change notification settings - Fork 4
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-details
web component
#29
base: develop
Are you sure you want to change the base?
Conversation
usa-details
web component
usa-details
web componentusa-details
web 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.
Update: I had originally imagined that the implementation would look something like this for single select "accordion" behavior:
<usa-details-group>
<usa-details name="example-details-group">
<details>
<summary>First Amendment</summary>
<div slot="details-body">Congress shall make no law respecting an establishment of religion, or prohibiting the free exercise thereof; or abridging the freedom of speech, or of the press; or the right of the people peaceably to assemble, and to petition the Government for a redress of grievances.</div>
</details>
</usa-details>
<usa-details name="example-details-group">
<details>
<summary>Second Amendment</summary>
<div slot="details-body">A well regulated Militia, being necessary to the security of a free State, the right of the people to keep and bear Arms, shall not be infringed.</div>
</details>
</usa-details>
</usa-details-group>
However, this structure isolated the two <details>
elements from each other in the shadow DOM and prevented the native name
attribute from working as expected. I updated the structure to this instead so that we could rely on the the native functionality of the <details>
element to toggle the component instead of writing custom JS:
<usa-details>
<details open="true" name="example-details-group">
<summary>First Amendment</summary>
<div slot="details-body">Congress shall make no law respecting an establishment of religion, or prohibiting the free exercise thereof; or abridging the freedom of speech, or of the press; or the right of the people peaceably to assemble, and to petition the Government for a redress of grievances.</div>
</details>
<details name="example-details-group">
<summary>Second Amendment</summary>
<div slot="details-body">A well regulated Militia, being necessary to the security of a free State, the right of the people to keep and bear Arms, shall not be infringed.</div>
</details>
</usa-details>
Curious to hear if anyone has concerns about this update.
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.
Nice job! It looks just like the existing accordion. My testing focused a lot on the functionality of the component.
I tested the following:
- Changing controls
- Updating detail attributes & content
- Comparing custom element properties to native details element
Can we add a basic example of how to use in the PR description? Additionally a preview link would help too.
PR comments use conventional comment style.
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: Are there any events we can trigger via JS on this component?
For example, <usa-banner>
can be toggled via toggle()
in console.
details-toggle-2024-07-05.at.13.17.58.webm
item1Summary: {name: "Item 1 - Summary text"}, | ||
item1Content: { name: "Item 1 - Panel content"}, | ||
item1Open: { name: "Item 1 - Open panel on load"}, | ||
item2Show: { name: "Include item 2 in preview"}, | ||
item2Summary: {name: "Item 2 - Summary text", if: { arg: 'item2Show' } }, | ||
item2Content: { name: "Item 2 - Panel content", if: { arg: 'item2Show' } }, | ||
item2Open: { name: "Item 2 - Open panel on load", if: { arg: 'item2Show' } }, | ||
item3Show: { name: "Include item 3 in preview"}, | ||
item3Summary: {name: "Item 3 - Summary text", if: { arg: 'item3Show' } }, | ||
item3Content: { name: "Item 3 - Panel content", if: { arg: 'item3Show' } }, | ||
item3Open: { name: "Item 3 - Open panel on load", if: { arg: 'item3Show' } }, |
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 (if-minor): These are marked as defaults, but they're only used in two variants.
It'd be nice to cleanly separate this. Some suggestions:
- Separate default strings from default controls. For example, default args would be reserved for args that are seen on most/every variant.
- Adding ability to automatically add any number of accordions (depending on LOE).
src/components/usa-details/index.js
Outdated
static properties = { | ||
bordered: { type: Boolean } | ||
} |
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 (non-blocking): Should open
be listed here?
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.
I was only listing the props that should be added to <usa-details>
here. In the current iteration, the open
attribute should be added to the the child <details>
element, so I left it off the properties list for now. Let me know if that should change!
src/components/usa-details/index.js
Outdated
|
||
template() { | ||
const classes = { | ||
"usa-details": 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: This should be on the element if it's not dynamic.
src/components/usa-details/index.js
Outdated
this.content.classList.add('usa-details__content'); | ||
|
||
return html` | ||
<details class="${classMap(classes)}" open="${this.open || nothing}" name="${this.name || nothing}"> |
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: In native disclosure, the open
attribute doesn't have a value. For example something like open="false"
will still keep the element open.
We should follow this pattern to keep things consistent.
The Details disclosure element - HTML: HyperText Markup Language | MDN
src/components/usa-details/index.js
Outdated
* @cssprop --theme-details-font-family - Sets the font family for the details element | ||
* @cssprop --theme-details-border-color - Sets the border width for the details element | ||
* @cssprop --theme-details-border-width - Sets the border color for the details element | ||
* @cssprop --theme-details-background-color - Sets the background color of the content panels | ||
* @cssprop --theme-details-summary-background-color - Sets the background color of the summary element |
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: I wonder if there's a way to show these component settings in storybook args.
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: Curious why didn't we use usa-accordion
SCSS here?
item1Summary: {name: "Item 1 - Summary text"}, | ||
item1Content: { name: "Item 1 - Panel content"}, | ||
item1Open: { name: "Item 1 - Open panel on load"}, | ||
item2Show: { name: "Include item 2 in preview"}, | ||
item2Summary: {name: "Item 2 - Summary text", if: { arg: 'item2Show' } }, | ||
item2Content: { name: "Item 2 - Panel content", if: { arg: 'item2Show' } }, | ||
item2Open: { name: "Item 2 - Open panel on load", if: { arg: 'item2Show' } }, | ||
item3Show: { name: "Include item 3 in preview"}, | ||
item3Summary: {name: "Item 3 - Summary text", if: { arg: 'item3Show' } }, | ||
item3Content: { name: "Item 3 - Panel content", if: { arg: 'item3Show' } }, | ||
item3Open: { name: "Item 3 - Open panel on load", if: { arg: 'item3Show' } }, |
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: It'd be nice if we could do this in a more intuitive way that wasn't focused on a fixed number of elements.
Possible control ideas
- A button in controls that toggles fields for header/content.
- An array of objects for header/body content.
- A textarea to input any number of
<details>
.
.usa-details__summary { | ||
/* TODO: use local files */ | ||
/* TODO: determine if background image is still the best way to add icons here */ | ||
background-image: url(https://designsystem.digital.gov/assets/img/usa-icons/add.svg); |
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 (non-blocking): Is it possible to override this? How would I set my own icon?
src/components/usa-details/index.js
Outdated
this.content.classList.add('usa-details__content'); | ||
|
||
return html` | ||
<details class="${classMap(classes)}" open="${this.open || nothing}" name="${this.name || nothing}"> |
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.
<details class="${classMap(classes)}" open="${this.open || nothing}" name="${this.name || nothing}"> | |
<details class="usa-details ${classMap(classes)}" open="${this.open || nothing}" name="${this.name || nothing}"> |
Converting to draft while experiment with theming and adding usa-accordion component. |
- Moved to separate PR to prevent scope creep
Summary
Created an alpha version of a
usa-details
web componentRelated issue
Closes #19
Preview link
Details component (Storybook)
Sample implementation
Reference
Details element (MDN)
Requirements notes (WIP)
Common component names
Details, Accordion, Collapse
Variants
Settings and CSS Variables
$theme-accordion-border-width
--theme-details-border-width
$theme-accordion-border-color
--theme-details-border-color
$theme-accordion-button-background-color
--theme-details-summary-background-color
$theme-accordion-background-color
--theme-details-background-color
$theme-accordion-font-family
--theme-details-font-family
Possibilities for additional customization:
Accessibility
See related accessibility tests
Attributes & Props
open="true"
<details>
open
attribute is present on the<details>
element, the content inside itsdetails-content
slot should be visible on load.name="[group name]"
<details>
name
attribute is present on the<details>
element, the<details>
elements with the samename
value will be treated as a single-select group and toggle each other.Slots
details-body
<div>
<div slot="details-body">[Accordion panel content]</div>
Expected interactions:
enter
,spacebar
, mouse click, or voice commandTest cases