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

Discovery: CDS copilot genereted Loader experiment#1664

Draft
helene-mccarron wants to merge 12 commits into
mainfrom
CDS-contribution-experiment
Draft

Discovery: CDS copilot genereted Loader experiment#1664
helene-mccarron wants to merge 12 commits into
mainfrom
CDS-contribution-experiment

Conversation

@helene-mccarron
Copy link
Copy Markdown

@helene-mccarron helene-mccarron commented Jun 24, 2025

Chromatic

https://CDS-contribution-experiment--65a6e2ed2314f7b8f98609d8.chromatic.com

Description

An experiment to convert the Clinical Design System component Loader into a web component.

Styles aren't fully implemented as part of this proof of concept but the component code is ready for review. It was generated using Copilot, after analyzing va-loading-indicator.

Please focus review on potential hallucinations and markedly erroneous patterns to help with refining the prompt.

Related tickets and links

Closes

Screenshots

Testing and review

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

@jamigibbs jamigibbs added the ignore-for-release Used if you want to ignore the PR in the generated release notes label Jun 24, 2025
@jamigibbs jamigibbs requested a review from Copilot June 25, 2025 15:51
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR provides a proof-of-concept conversion of the Clinical Design System Loader component into a web component, generated using Copilot. Key changes include the addition of new CSS styling, end-to-end tests for functionality and accessibility, and the generated web component code for the loader.

Reviewed Changes

Copilot reviewed 4 out of 6 changed files in this pull request and generated 3 comments.

File Description
packages/web-components/src/components/va-loader/va-loader.css Added CSS styling for the loader including animation, layout, and visual details.
packages/web-components/src/components/va-loader/test/va-loader.e2e.ts Added new e2e tests for rendering, custom labels, dynamic text changes, and accessibility checks.
packages/web-components/src/components/va-loader/loader-generated.tsx Introduced the web component implementation with rotation logic and dynamic label updates.
packages/storybook/stories/va-loader.stories.tsx Added Storybook stories to demonstrate various loader configurations.

top: 45%;
left: 30%;
transform: translate(-50%, -50%);
font-family: Source Sans Pro Web, Helvetica Neue, Helvetica, Arial, sans
Copy link

Copilot AI Jun 25, 2025

Choose a reason for hiding this comment

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

The generic font family should be 'sans-serif' instead of 'sans' to ensure proper font rendering.

Suggested change
font-family: Source Sans Pro Web, Helvetica Neue, Helvetica, Arial, sans
font-family: Source Sans Pro Web, Helvetica Neue, Helvetica, Arial, sans-serif

Copilot uses AI. Check for mistakes.
Comment thread packages/web-components/src/components/va-loader/va-loader.css Outdated
.vacds-loader {
position: relative;
width: 12.5rem;
height: 12.5rem
Copy link

Copilot AI Jun 25, 2025

Choose a reason for hiding this comment

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

[nitpick] For stylistic consistency, add a trailing semicolon after the height property.

Suggested change
height: 12.5rem
height: 12.5rem;

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Contributor

@jamigibbs jamigibbs left a comment

Choose a reason for hiding this comment

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

There is functional base here that could continue to be built on but this work would need a lot more human attention to detail related to building out the style and Storybook previews.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

suggestion (blocking): The name of this file should be name of the component va-loader.tsx which would follow the current component-library pattern.

/**
* @componentName Loader
* @maturityCategory review
* @maturityLevel development
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

suggestion (blocking): The maturityCategory and maturityLevel should follow these maturity scale values

height: 12.5rem
}

.vacds-loader-border {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

suggestion (blocking): Class naming should follow the naming conventions outlined here: https://design.va.gov/about/naming-conventions/

position: absolute;
top: 0;
left: 0;
border: 1.25rem solid #f0f0f0;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

suggestion (blocking): Colors should use VADS color tokens where possible. New colors should be evaluated for addition to the design system CSS-Library

SetFocus.args = { ...defaultArgs, 'set-focus': true };

export const EnableAnalytics = Template.bind(null);
EnableAnalytics.args = { ...defaultArgs, 'enable-analytics': true };
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

suggeston (blocking): There is no enable-analytics property on the component so this story should not have been added. This could be an issue with the va-loader story too but even so, we don't want the same problem to be duplicated.


const defaultArgs = {
'message': 'Loading your application...',
'label': 'Loading',
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

label and message are declared but not being used.

Copy link
Copy Markdown
Contributor

@jamigibbs jamigibbs left a comment

Choose a reason for hiding this comment

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

Testing locally, the loader looks like this:

Screen.Recording.2025-07-02.at.8.31.29.AM.mov

I'm not familiar with the VACDS version of this component but is that acceptable for your use? I suspect it still needs some styling.

I'm also not sure that this "Custom Position" feature is working. It looks exactly the same as the other stories:

Screenshot 2025-07-02 at 8 36 05 AM


'Loader': {
guidanceHref: 'loader',
guidanceName: 'Loader',
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This file additional-docs.js is only for deprecated React components and styling classes from CSS-Library (previously Formation). It's not used for web components so this should be removed.

/**
* @componentName Loader
* @maturityCategory don't use
* @maturityLevel proposed
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This should be:

 * @maturityCategory caution
 * @maturityLevel candidate

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Thanks!

@humancompanion-usds
Copy link
Copy Markdown
Contributor

I had thought that one of the goals of revising the va-loading-indicator was to make it more accessible as evidenced by this discussion. They goal there appeared to be use of the prefers-reduced-motion CSS media feature. Is that still a goal? Otherwise, I'm not sure we need another loading indicator.

That said, if the purpose of the experiment was just to see if you could convert a component of a certain framework to a web-component using Copilot then that seems to have had mitigated success. We've not yet arrived at a series of prompts for converting to web components because we are generally starting from scratch or from a web-component contribution.

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

Labels

ignore-for-release Used if you want to ignore the PR in the generated release notes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants