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

fix: replace card button with link #2750

Merged

Conversation

klalicki
Copy link
Contributor

@klalicki klalicki commented Feb 7, 2024

Summary

This PR replaces <Button> components with <Link> components styled to look like buttons in the Storybook files for the Card component. Adds styling-related props from the original <Button> components (outline, secondary) via the corresponding CSS classes from USWDS - ie .usa-button--secondary
This PR also adds an optional allowSpacebarActivation prop to the Link component. When this prop is set to true the link can be activated with the spacebar in addition to the Enter key.

Related Issues or PRs

How To Test

Navigate to Components > Card > Card Examples, Components > Card > Card Tests, and Components > Typography > Link > Styled As Button in Storybook.

For each button:

  • navigate to it via keyboard
  • confirm that it uses an <a> element instead of a <button>
  • confirm that pressing the spacebar activates the link (will jump to top of page)

Note - visual regression testing will require review - this PR adds additional text to the story for Link Styled As A Button specifying that it can be activated with the spacebar.

Screenshots (optional)

Closes #2696

@klalicki klalicki requested review from a team as code owners February 7, 2024 22:39
@klalicki
Copy link
Contributor Author

klalicki commented Feb 7, 2024

A couple of comments/questions for @werdnanoslen / whoever else at Truss is reading this:

  • The new code adds a onKeyDown event handler to the a element returned by <Link>. This event listener would be overridden if a developer passes in their own custom onKeyDown handler. A developer probably shouldn't pass a keyboard event listener to this component, but is this a scenario that I should account for? I could add the event listener with addEventListener in useEffect if so.
  • The new code currently only adds the keyboard listener to the component when the default element <a> is being used. If a developer is using an asCustom component with Link, the keyboard listener is not added. My thought behind this was that you can't guarantee that the component passed to this one for asCustom will actually accept an event listener as a prop. Should I pass the event listener to the custom component anyway? I imagine this would most often be used for frameworks with routing like Next.js (whose Link component does accept event listeners)
  • possibly unrelated to the scope of the original issue - in order for the <Link> styled as a button to look correct, I have to also give it variant="unstyled" to get rid of the standard link text color, or else the default link text color overrides the white text color from .usa-button. Should the combination of characteristics for Link styled as a Button (spacebar handler, variant="unstyled", and class of "usa-button") be combined into one property ie <Link isButton href={"..."}> or is it preferable to keep the styling and spacebar functionality separate?

werdnanoslen
werdnanoslen previously approved these changes Feb 8, 2024
Copy link
Member

@werdnanoslen werdnanoslen left a comment

Choose a reason for hiding this comment

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

Approving for design, accessibility, and meeting the issue AC. I'll defer to someone else on code review and first two questions above.

@werdnanoslen
Copy link
Member

possibly unrelated to the scope of the original issue - in order for the styled as a button to look correct, I have to also give it variant="unstyled" to get rid of the standard link text color, or else the default link text color overrides the white text color from .usa-button. Should the combination of characteristics for Link styled as a Button (spacebar handler, variant="unstyled", and class of "usa-button") be combined into one property ie <Link isButton href={"..."}> or is it preferable to keep the styling and spacebar functionality separate?

I think the way you've done it maximizes user control and exposes underlying components in a clear way that isButton would obfuscate too much imo. So I like this approach!

@werdnanoslen werdnanoslen requested a review from a team February 8, 2024 21:05
Copy link
Contributor

@brandonlenz brandonlenz left a comment

Choose a reason for hiding this comment

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

Thanks for this! Personally good to approve, but would love to see a default value for the new allowSpacebarActivation prop, just to be super explicit.

Please ping me cause I'll need to re-review at that point anyways!

src/components/Link/Link.tsx Outdated Show resolved Hide resolved
<Link
className="usa-button"
variant="unstyled"
allowSpacebarActivation
Copy link
Contributor

Choose a reason for hiding this comment

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

As someone who hasn't tried using spacebar to activate an anchor tag (but have used it for buttons) this was a fun rabbit hole (w3c/aria#1701). I think this is a fair feature.

<a className={classes} {...linkProps}>
<a
className={classes}
{...(allowSpacebarActivation && { onKeyDown: handleKeyDown })}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggestion, non blocking: You could instead use allowSpacebarActivation inside the handleKeyDown method to decide whether or not to do anything. I think this is fine though, just conditionally spreading props like this looks a little wonky at first.

@@ -33,6 +34,13 @@ export function isCustomProps<T>(
): props is CustomLinkProps<T> {
return 'asCustom' in props
}
// keyboard handler for 'link as a button'
const handleKeyDown = (e: React.KeyboardEvent) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

I was hoping typing this as const handleKeyDown: React.KeyboardEventHandler<HTMLAnchorElement> would maybe avoid the need to cast e.target, but alas, it does not. So IMO, this is fine.

klalicki and others added 2 commits March 1, 2024 16:33
@werdnanoslen werdnanoslen merged commit 59d5f14 into trussworks:main Mar 4, 2024
7 checks passed
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

Successfully merging this pull request may close these issues.

Replace Card's Buttons with Links
3 participants