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

Establish a verification checklist #198

Closed
33 tasks
natalia-fitzgerald opened this issue Oct 10, 2023 · 7 comments
Closed
33 tasks

Establish a verification checklist #198

natalia-fitzgerald opened this issue Oct 10, 2023 · 7 comments
Assignees

Comments

@natalia-fitzgerald
Copy link

natalia-fitzgerald commented Oct 10, 2023

@meissadia @shindigira @billhimmelsbach

I would like to establish a verification checklist for our components so that we can share in the responsibilities of reviewing and verifying. Please review provide feedback or additions.

Verification checklist

1. Verify the CFPB Design System (React) component against the CFPB Design System

For existing components

  • Has component intro text copied from the DS page
  • Has source link to component's DS page (ex - Source: https://cfpb.github.io/design-system/components/banner-notification)
  • Each DS variant is implemented as a separate story
    • Story name should be sentence case (ex. List Link => List link)
    • Naming is consistent with the DS
    • Same component names, same placeholder text, etc.
  • Order of stories matches between DS/DSR
  • Component is built correctly
    • Visually compare DSR implementation to DS
    • Verify that React renders the HTML markup the same as in the DS (if at all possible)
    • Verify that DS classes (organisms, molecules, atoms) are used, as opposed to styles defined in DSR
  • Check to see if the applied CSS styles match the Specs in the DS if applicable (message @natalia-fitzgerald if it does not)
  • Manual visual review has been conducted and component has passed this review

For new components

  • All steps for existing components plus the following steps
  • The new component has been added to the CFPB Design System OR
  • The CFPB Design System maintainers have been alerted that there is a new component that must be added to the system
  • Documentation has been written for the new component (this is not a blocker for moving a component to verified)

2. Run accessibility checks

  • Component is keyboard-friendly (navigable with tab, space, enter, arrow keys, etc.)
  • Component does not introduce new errors or warnings in WAVE
  • Component is screen reader friendly (screen reader testing demo)
    • Is all the meaningful visual information and text of the component conveyed by the screen reader? (text, non-decorative image descriptions, etc.)
    • When interacting with the component using a screen reader, do you have all the information you need to use it? (selected vs unselected, button vs link, expanded vs collapsed, etc.)
    • Does the component have similar screen reader behavior as the sample components in WCAG, the CFPB design system, WebAIM, or similar accessibility examples?
  • For reference: CFPB manual web accessibility audit

3. Verify component unit tests

  • Verify component unit tests are implemented and passing - 85% or greater (ex: yarn vitest Button)

4. Prepare a PR for Code Review

  • Create a PR with the name: design(XXXX): final design review for XXXX component
  • Copy this verification checklist that's almost complete into the PR description

5. Conduct Code PR review

  • Submit PR with any necessary changes for code review by frontend devs

6. Conduct Design PR review

7. Verify component

  • Merge when design review completed to finish component verification 🎉
@meissadia
Copy link
Contributor

@natalia-fitzgerald Looks great!

  • The component is successfully integrated into the Small business lending filing platform (or prototype) or other CFPB web app (is this a necessary step or is it enough that it's in Storybook?)

    I think Storybook is a standalone space where we can verify that components work. Stories should be implemented in a way that reviewers can validate functionality, accessibility,

  • Visual regression testing has been conducted
    I don't yet know enough about how to use Chromatic (and I think the integration is broken again), so I would vote to skip this for now.

@billhimmelsbach
Copy link
Contributor

This is looking great @natalia-fitzgerald!

  • The component is successfully integrated into the Small business lending filing platform (or prototype) or other CFPB web app (is this a necessary step or is it enough that it's in Storybook?)

I have mixed feelings if the component needs to be used before it can be validated? When I've integrated new components from component libraries, I often do have to make small tweaks, but I don't think that should prevent it from being validated?

  • Component is screen reader friendly

How have people been verifying this so far? I usually use VoiceOver on my mac, but I've seen significant differences when testing things out in JAWS or Narrator on windows at previous gigs.

It wouldn't have to be part of this more high level definition of done document, but it might be worth breaking this task down into smaller tasks when we do our code reviews (Verify that meaningful visible text is conveyed, links have descriptive text, etc.). Maybe just by linking to a generic accessibility checklist that includes a screen reader section like this one or just the WCAG docs?

For new components:

Could we add something about testing coverage here to this section? I saw in sbl-project's definition of done a generic >85% code coverage requirement, which while an arbitrary benchmark, might be nice to have here.

Thanks Natalia!

@natalia-fitzgerald
Copy link
Author

Thanks for your feedback @billhimmelsbach @meissadia

  • I'll remove this item: "The component is successfully integrated into the Small business lending filing platform (or prototype) or other CFPB web app" but we should capture somewhere that there could be changes when a component is finally used in the real world. I see this as a test of the component.
  • For accessibility checks let's follow the CFPB manual web accessibility audit and [Lighthouse automated audit] (https://cfpb.github.io/design-system/guidelines/accessibility-audit-tools#lighthouse-automated-audit-1)
  • At what phase of the process would we like to run the accessibility audit? Component level? Page level? If we wait for the page level we should make a short list of quick things to check at the component phase.
  • @billhimmelsbach Can you elaborate on the code coverage benchmark? Or do other devs have thoughts on that?
  • I'll remove the visual regression testing but if we are doing manual visual review instead I'd like to be sure we are working from the CFPB Design System component and styles as much as possible unless there is a justification for deviating. When there are changes we wish to make we should push them to the CFPB Design System and have them feed back out to the DSR.

@billhimmelsbach
Copy link
Contributor

Great WCAG guidelines! Sounds good to me for whatever manual testing (screen reader / tab navigation) we want to do.

... and [Lighthouse automated audit] (https://cfpb.github.io/design-system/guidelines/accessibility-audit-tools#lighthouse-automated-audit-1)

I'm down for including lighthouse, but I think we already run Storybook's A11y extension which is based on axe-core's accessibility tooling? It's the one that pops up in the accessibility tab on the bottom of storybook components.

Might be worth just tossing whatever we decide into a GitHub action too, so that it just runs on PRs.

At what phase of the process would we like to run the accessibility audit? Component level? Page level? If we wait for the page level we should make a short list of quick things to check at the component phase.

Probably the component level for me? For example, best to avoid things like creating a <div> soup instead of using semantic html when building the component itself, rather than having to rebuild the component later when trying to use it?

@billhimmelsbach Can you elaborate on the code coverage benchmark? Or do other devs have thoughts on that?

Yeah, when building out the components, devs have been writing automated tests to verify the functionality of the component, ensure future devs don't mess things up when doing refactoring, etc. Here's some good tests @meissadia wrote for pagination, for example.

For the code coverage metric, >85% would mean that at least that amount of the lines of code should be covered by some kind of automated test. For a lot of the simpler components in the library so far, rendering the component alone is enough to reach this metric. For more complicated components, it's an arbitrary but nice benchmark to set, just so we make sure our code is well tested for future refactors and what not.

I'm happy to move the percentage up or down depending on how people feel. 👍

@billhimmelsbach
Copy link
Contributor

I don't yet know enough about how to use Chromatic (and I think the integration is broken again), so I would vote to skip this for now.

I haven't used Chromatic much either @meissadia, but I put up a PR this morning to hopefully at least get the integration working again. 🤞 I have used other visual regression tooling, so might be worth fixing up once our components are more stable.

@natalia-fitzgerald natalia-fitzgerald changed the title Establish a validation checklist Establish a verification checklist Oct 12, 2023
@meissadia
Copy link
Contributor

Verifying DSR component

  • Has component intro text copied from the DS page
  • Has source link to component's DS page (ex - Source: https://cfpb.github.io/design-system/components/banner-notification)
  • Each DS variant is implemented as a separate story
    • Story name should be sentence case (ex. List Link => List link)
    • Naming is consistent with DS
      • Same component names, same placeholder text, etc.
  • Order of stories matches between DS/DSR
  • Component is built correctly
    • Visually compare DSR implementation to DS
    • Verify that DS classes (organisms, molecules, atoms) are used, as opposed to styles defined in DSR
    • Verify component unit tests are implemented and passing (ex: yarn vitest Button)

@natalia-fitzgerald
Copy link
Author

We are closing this one out since we are using the checklist.

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

No branches or pull requests

4 participants