Skip to content

Align sign in button with site design system#61

Merged
sethwebster merged 3 commits into
react-foundation-dev:mainfrom
pchalupa:fix/sign-in-button-style
May 13, 2026
Merged

Align sign in button with site design system#61
sethwebster merged 3 commits into
react-foundation-dev:mainfrom
pchalupa:fix/sign-in-button-style

Conversation

@pchalupa

@pchalupa pchalupa commented May 8, 2026

Copy link
Copy Markdown
Contributor

Summary

The "Sign in" button in the header used a one-off class string instead of the design system's primary button primitive. That ad-hoc styling diverged from the rest of the site's primary CTAs and, as a side effect, produced a low-contrast text color in dark mode. This PR replaces the custom Link with RFDS.ButtonLink, aligning the sign-in CTA with the design system and removing the contrast drift permanently.


Root Cause (for bugs)

Wrong color on the button's text.


Solution

Replaced the Next.js Link component with the RFDS.ButtonLink component.


Scope

  • This PR has a single concern
  • Refactors are directly required for this change
  • No unrelated formatting or cleanup is included

Test Plan

  • Added or updated tests for the changed behavior
  • Confirmed the test fails without the fix, when practical
  • Verified tests pass with the fix
  • Regression tested the original scenario
  • Manually verified UI behavior if applicable

Describe specific scenarios tested:

The sign-in button displays with white text in dark mode.


Screenshots (if UI)

Before

Screenshot 2026-05-08 at 11 51 24 Screenshot 2026-05-08 at 11 51 20

After

Screenshot 2026-05-08 at 11 51 34 Screenshot 2026-05-08 at 11 51 39
Screen.Recording.2026-05-08.at.11.26.29-compressed.mp4

Checklist

  • Code follows existing patterns
  • No unnecessary abstractions
  • Tests included and passing
  • No console logs or debug code
  • No unresolved TODOs introduced
  • CI passing

@vercel

vercel Bot commented May 8, 2026

Copy link
Copy Markdown
Contributor

@pchalupa is attempting to deploy a commit to the Pro Team Team on Vercel.

A member of the Team first needs to authorize it.

Co-authored-by: Copilot <copilot@github.com>
Signed-off-by: pchalupa <chalupa.petr93@gmail.com>
@pchalupa pchalupa force-pushed the fix/sign-in-button-style branch from 53eda7b to 44e0a04 Compare May 8, 2026 10:03
href="/api/auth/signin"
className="rounded-full border border-border bg-primary px-4 py-2 text-xs font-semibold uppercase tracking-[0.2em] text-primary-foreground transition hover:bg-primary/90"
>
<RFDS.ButtonLink href="/api/auth/signin" size="sm">

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This swap does fix the contrast token, but it also changes the sign-in CTA typography/spacing/shadow (text-xs uppercase tracking -> default ButtonLink sm). Please either preserve the previous shape more narrowly or document/test the broader visual restyle so the scope matches the PR description.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Visually, it makes sense to use a consistent style for the primary button, so that "Get involved" and "Sign in" buttons now look more unified.

@dougbot-agent dougbot-agent left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks for the contribution — good direction here.

A few things we need before merging:

  1. Tests
    We need explicit test coverage for the unauthenticated header CTA change. This is a user-visible behavior fix, and the current PR leaves the test-plan boxes unchecked.
  2. Scope / description
    The implementation does more than change text color: it swaps in the default RFDS button styling, which also changes sizing/typography/shadow. Please either narrow the change to the contrast fix or update the PR/tests to cover the broader restyle intentionally.
  3. Verification
    I pulled the branch locally and npm run lint -- src/components/layout/header.tsx plus npx tsc --noEmit both pass. CI is mostly green here; the Vercel check is just waiting on authorization, so the blocking feedback is about coverage/scope rather than build health.

Once those are addressed, we can take another pass.

@dougbot-agent

Copy link
Copy Markdown
Collaborator

@sethwebster maintainer note: the current Vercel check is blocked on team authorization rather than contributor code. Contributor-side review feedback is already on the PR (tests/scope), but if those changes land this repo-side authorization will still need attention.

@pchalupa pchalupa changed the title Fix sign in button text color Unify button style May 12, 2026
@pchalupa pchalupa changed the title Unify button style Align sign in button with site design system May 12, 2026
@pchalupa

Copy link
Copy Markdown
Contributor Author

Thanks for the contribution — good direction here.

A few things we need before merging:

  1. Tests
    We need explicit test coverage for the unauthenticated header CTA change. This is a user-visible behavior fix, and the current PR leaves the test-plan boxes unchecked.
  2. Scope / description
    The implementation does more than change text color: it swaps in the default RFDS button styling, which also changes sizing/typography/shadow. Please either narrow the change to the contrast fix or update the PR/tests to cover the broader restyle intentionally.
  3. Verification
    I pulled the branch locally and npm run lint -- src/components/layout/header.tsx plus npx tsc --noEmit both pass. CI is mostly green here; the Vercel check is just waiting on authorization, so the blocking feedback is about coverage/scope rather than build health.

Once those are addressed, we can take another pass.

  1. There is no change in behavior, only a visual change. I think manual testing is fine here.
  2. The PR has been renamed to better describe the change.

@pchalupa pchalupa requested a review from dougbot-agent May 12, 2026 19:26
@dougbot-agent

Copy link
Copy Markdown
Collaborator

Thanks for the follow-up. The title update helps, but the blocking point is still automated coverage.

For this repo, user-visible fixes need a regression check even when the change is “just visual,” because otherwise we have no durable way to catch the same contrast/style regression later. Manual verification is useful, but it is not enough on its own for merge.

What would unblock this from my side:

  1. Add a small automated regression check for the unauthenticated header CTA (for example, extend an existing Playwright flow to assert the sign-in CTA renders with the intended button treatment / visible contrast in the header state you changed).
  2. Keep the scope aligned with the description. Right now the implementation is still broader than a text-color tweak because it swaps in full RFDS.ButtonLink styling. That is fine if intentional, but then the PR body + test coverage should reflect the broader restyle explicitly.

Once that is in place, I’m happy to take another pass.

Signed-off-by: pchalupa <chalupa.petr93@gmail.com>
@pchalupa pchalupa force-pushed the fix/sign-in-button-style branch from 0d808f9 to 69d6114 Compare May 13, 2026 11:40
@pchalupa

Copy link
Copy Markdown
Contributor Author

Thanks for the follow-up. The title update helps, but the blocking point is still automated coverage.

For this repo, user-visible fixes need a regression check even when the change is “just visual,” because otherwise we have no durable way to catch the same contrast/style regression later. Manual verification is useful, but it is not enough on its own for merge.

What would unblock this from my side:

  1. Add a small automated regression check for the unauthenticated header CTA (for example, extend an existing Playwright flow to assert the sign-in CTA renders with the intended button treatment / visible contrast in the header state you changed).
  2. Keep the scope aligned with the description. Right now the implementation is still broader than a text-color tweak because it swaps in full RFDS.ButtonLink styling. That is fine if intentional, but then the PR body + test coverage should reflect the broader restyle explicitly.

Once that is in place, I’m happy to take another pass.
Changes:

  1. Added a Playwright visual screenshot assertion of the page header in the unauthenticated state.
  2. Updated PR description to better frame the scope.

@dougbot-agent dougbot-agent left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks for the update — this is closer.

The remaining blocker on my side is the new regression coverage itself:

  1. CI is currently failing because the PR only adds a Darwin snapshot (header-sign-in-chromium-darwin.png), while the GitHub Actions runner expects the Linux baseline (header-sign-in-chromium-linux.png).
  2. Please either regenerate/commit the Linux snapshot used in CI, or switch this to a platform-agnostic assertion that validates the intended sign-in CTA treatment without depending on a macOS-only artifact.
  3. Once that test is green, I’m happy to take another pass.

Separately, the Vercel authorization failure still looks maintainer-side rather than contributor-side.

Signed-off-by: pchalupa <chalupa.petr93@gmail.com>
@pchalupa pchalupa force-pushed the fix/sign-in-button-style branch from 44eb5c0 to 72516ec Compare May 13, 2026 12:33
@pchalupa

Copy link
Copy Markdown
Contributor Author

Thanks for the update — this is closer.

The remaining blocker on my side is the new regression coverage itself:

  1. CI is currently failing because the PR only adds a Darwin snapshot (header-sign-in-chromium-darwin.png), while the GitHub Actions runner expects the Linux baseline (header-sign-in-chromium-linux.png).
  2. Please either regenerate/commit the Linux snapshot used in CI, or switch this to a platform-agnostic assertion that validates the intended sign-in CTA treatment without depending on a macOS-only artifact.
  3. Once that test is green, I’m happy to take another pass.

Separately, the Vercel authorization failure still looks maintainer-side rather than contributor-side.

Good catch! A Linux screenshot has been added.

@pchalupa pchalupa requested a review from dougbot-agent May 13, 2026 12:37

@dougbot-agent dougbot-agent left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This looks solid.

  • Clear fix
  • Tests cover behavior
  • Implementation is clean
    Approved ✅

@sethwebster sethwebster merged commit 690e988 into react-foundation-dev:main May 13, 2026
6 of 7 checks passed
@pchalupa pchalupa deleted the fix/sign-in-button-style branch May 13, 2026 14:01
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.

3 participants