Skip to content
This repository is currently being migrated. It's locked while the migration is in progress.

va-accordion: move border prop from accordion-item to accordion#2079

Open
powellkerry wants to merge 6 commits into
mainfrom
2513-move-border-prop-to-accordion
Open

va-accordion: move border prop from accordion-item to accordion#2079
powellkerry wants to merge 6 commits into
mainfrom
2513-move-border-prop-to-accordion

Conversation

@powellkerry
Copy link
Copy Markdown
Contributor

@powellkerry powellkerry commented Apr 21, 2026

Chromatic

https://2513-move-border-prop-to-accordion--65a6e2ed2314f7b8f98609d8.chromatic.com

Summary

Update to move the bordered prop to va-accordion and deprecate it from va-accordion-item. This change enforces borders being synchronized in a component instance. Backwards compatibility was built into these changes.

Description

The bordered prop was only available on the va-accordion-item component, this allowed for implementations to have borders on some items and not others. This change makes it so that borders are applied uniformly across an implementation. Backwards compatibility has been maintained by adding logic to check for the bordered attribute on items and applying the border if the attribute is found. The current prop has also been maintained for backwards compatibility.

Related tickets and links

Link to any related issues, PRs, Slack conversations, or anything else relevant to documenting the changes.

Closes 2513

Screenshots

No visual changes, storybook shows the prop was moved to va-accordion.

Screenshot 2026-04-21 at 9 47 32 AM

Testing and review

Provide any testing instructions or review steps as needed.

Approvals

See the QA Checklists section below for suggested approvals. Use your best judgment if additional reviews are needed. When in doubt, request a review.

Approval groups

Add approval groups to the PR as needed:

QA checklists

Use the QA checklists below as guides, not rules. Not all checklists will apply to every PR but there could be some overlap.

In all scenarios, changes should be fully tested by the author and verified by the reviewer(s); functionality, responsiveness, etc.

✨ New Component Added
  • The PR has the minor label
  • The component matches the Figma designs.
  • All properties, custom events, and utility functions have e2e and/or unit tests
  • A new Storybook page has been added for the component
  • Tested in all VA breakpoints.
  • Chromatic UI Tests have run and snapshot changes have been accepted by the design reviewer
  • Tested in vets-website using Verdaccio
  • Engineering has approved the PR
  • Design has approved the PR
  • Accessibility has approved the PR
🌱 New Component Variation Added
  • The PR has the minor label
  • The variation matches its Figma design.
  • Any new properties, custom events, or utility functions have e2e and/or unit tests
  • A new story has been added to the component's existing Storybook page
  • Any Chromatic UI snapshot changes have been accepted by a design reviewer
  • Tested in vets-website using Verdaccio
  • Engineering has approved the PR
  • Design has approved the PR
🐞 Component Fix
  • The PR has the patch label
  • Any new properties, custom events, or utility functions have e2e and/or unit tests
  • Any markup changes are evaluated for impact on vets-website.
    • Will any vets-website tests fail from the change?
  • Any Chromatic UI snapshot changes have been reviewed and approved by a designer if necessary
  • Engineering has approved the PR
♿️ Component Fix - Accessibility
  • The PR has the patch label
  • Any new properties, custom events, or utility functions have e2e and/or unit tests
  • Any Chromatic UI snapshot changes have been reviewed and approved by a designer if necessary
  • Engineering has approved the PR
  • Accessibility has approved the PR
🚨 Component Fix - Breaking API Change
  • The PR has the major label
  • vets-website and content-build have been evaluated to determine the impact of the breaking change
  • Any new properties, custom events, or utility functions have e2e and/or unit tests
  • Any Chromatic UI snapshot changes have been reviewed and approved by a designer if necessary
  • Tested in vets-website using Verdaccio
  • Engineering has approved the PR
🔧 Component Update - Non-Breaking API Change
  • The PR has the minor label
  • Any new properties, custom events, or utility functions have e2e and/or unit tests
  • Any Chromatic UI snapshot changes have been reviewed and approved by a designer if necessary
  • Engineering has approved the PR
📖 Storybook Update
  • The PR has the ignore-for-release label
  • Any Chromatic UI snapshot changes have been reviewed and approved by a designer if necessary
  • Engineering has approved the PR
🎨 CSS-Library Update
  • The PR has the css-library label
  • vets-website and content-build have been checked to determine the impact of any breaking changes
  • Engineering has approved the PR

@powellkerry powellkerry requested a review from a team as a code owner April 21, 2026 15:49
@powellkerry powellkerry added the minor For a minor Semantic Version change label Apr 21, 2026
Copy link
Copy Markdown
Contributor

@RyanMunsch RyanMunsch left a comment

Choose a reason for hiding this comment

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

LGTM. This improvement makes a lot more sense than requiring bordered on each individual item.

Copy link
Copy Markdown
Contributor

@ediiotero ediiotero left a comment

Choose a reason for hiding this comment

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

LGTM


// Enforce all-or-none bordered rendering within the same accordion while keeping
// backward compatibility for legacy item-level bordered usage.
private updateResolvedBordered() {
Copy link
Copy Markdown
Contributor

@jamigibbs jamigibbs Apr 22, 2026

Choose a reason for hiding this comment

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

question: Have we considered doing the border management in the parent component va-accordion instead of in the child component here? I'm wondering if that would have a performance improvement with a single MutationObserver and also make it easier for us to cleanup any backwards compatibility work (ie. maybe just removing a single prop in the child).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

minor For a minor Semantic Version change

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Accordion - Border prop and Storybook

4 participants