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

design(Link): final design review for Link component #228

Merged
merged 2 commits into from
Nov 7, 2023

Conversation

meissadia
Copy link
Contributor

@meissadia meissadia commented Oct 31, 2023

Closes #215

This PR is to get the Design team's (@natalia-fitzgerald) final approval.

@billhimmelsbach @shindigira Please ignore

Changes

  • Moves Link component to Verified status

Verification checklist

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

  • 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 DS classes (organisms, molecules, atoms) are used, as opposed to styles defined in DSR
  • Manual visual review has been conducted and component has passed this review

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

Verify component unit tests

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

Conduct Code PR review

  • Submit PR with any necessary changes for code review by frontend devs and copy completed checklist above into PR description

Conduct Design PR review

Verify component

  • Merge when design review completed to finish component verification 🎉

Copy link

netlify bot commented Oct 31, 2023

Deploy Preview for cfpb-design-system-react ready!

Name Link
🔨 Latest commit 057498a
🔍 Latest deploy log https://app.netlify.com/sites/cfpb-design-system-react/deploys/654ac65a74886f00085f1ed4
😎 Deploy Preview https://deploy-preview-228--cfpb-design-system-react.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

@natalia-fitzgerald natalia-fitzgerald left a comment

Choose a reason for hiding this comment

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

@meissadia
The component is looking good! I made some text updates to the CFPB Design System: Links page that I'd like to incorporate into the DSR page before we move to verified.

New intro text:
Links are navigational elements that connect users to other locations, either on the current page or to a different page or site. In contrast, buttons are used to signal important actions.

Stories

  • Inline links
  • List link Call-to-action links
    • Change code example for the links to "List link 1" "List link 2"
  • Destructive links
    • Change code example link text to "Destructive link" (remove the word "Sample"
  • Link with iconStandard link with icon
  • Non-wrapping link with icon icon links
  • Jump link
    • Change code example for the link to "Jump link" (remove the word "Default")
  • Jump link with icon on the left
  • Printed links (Should we add this to the DSR?)

@billhimmelsbach billhimmelsbach changed the title task: [Link] Move to Verified state design(Link): Move to Verified state Nov 6, 2023
@billhimmelsbach billhimmelsbach added the design needs design review for verification label Nov 6, 2023
@billhimmelsbach
Copy link
Contributor

billhimmelsbach commented Nov 6, 2023

Heyo @meissadia! I just changed the title, added the verification checklist to the description, and added a label just to make it align with these steps I wrote up last week. We can discuss/change the steps if we want to, but just wanted to make the outstanding PRs consistent for now. 👍

@billhimmelsbach billhimmelsbach changed the title design(Link): Move to Verified state design(Link): final design review for Link component Nov 6, 2023
@natalia-fitzgerald natalia-fitzgerald self-requested a review November 7, 2023 16:19
Copy link

@natalia-fitzgerald natalia-fitzgerald left a comment

Choose a reason for hiding this comment

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

Hey @meissadia - I accidentally approved thinking this was the Tagline. Can I re-review?

@natalia-fitzgerald natalia-fitzgerald self-requested a review November 7, 2023 16:59
Copy link

@natalia-fitzgerald natalia-fitzgerald left a comment

Choose a reason for hiding this comment

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

  • Change intro text to:
    Links are navigational elements that connect users to other locations, either on the current page or to a different page or site. In contrast, buttons are used to signal important actions.

  • Change "Call-to-action links" to "List links"

  • Change live code text to: "List link 1" and "List link 2"

  • Change "Sample destructive link" to "Destructive link"

  • Change "Standard link with icon" to "Link with icon"

  • Change "Non-wrapping icon links" to "Non-wrapping link with icon"

@natalia-fitzgerald
Copy link

@meissadia
Also: Change "Default jump link" to "Jump link" - in the live code example

@natalia-fitzgerald
Copy link

natalia-fitzgerald commented Nov 7, 2023

The changes just requested are the same ones I made here: #228 (review)

I also asked about whether we should include the "Printed links"

@meissadia meissadia force-pushed the 215-link-move-to-verified branch 3 times, most recently from 19e4187 to c717ae3 Compare November 7, 2023 23:11
@meissadia meissadia force-pushed the 215-link-move-to-verified branch from c717ae3 to 057498a Compare November 7, 2023 23:20
@meissadia meissadia enabled auto-merge (squash) November 7, 2023 23:26
@meissadia meissadia disabled auto-merge November 7, 2023 23:26
Copy link

@natalia-fitzgerald natalia-fitzgerald left a comment

Choose a reason for hiding this comment

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

Looks great!

@meissadia meissadia merged commit 52e1758 into main Nov 7, 2023
5 checks passed
@meissadia meissadia deleted the 215-link-move-to-verified branch November 7, 2023 23:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
design needs design review for verification
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Task] Verify Links Component
3 participants