Skip to content

fix: fix button style on Hero#270

Open
117Isabell wants to merge 1 commit intoWomen-Coding-Community:mainfrom
117Isabell:fix/button-style-on-hero
Open

fix: fix button style on Hero#270
117Isabell wants to merge 1 commit intoWomen-Coding-Community:mainfrom
117Isabell:fix/button-style-on-hero

Conversation

@117Isabell
Copy link
Copy Markdown
Contributor

Description

Updates the home page hero “Join our Slack” control from a plain underlined Link to the shared LinkButton component with an outlined style so it matches design: pill shape, M3 outline border (#71787E), primary label colour, no underline, and label typography aligned with Figma (14px / medium weight / line-height / letter-spacing).

LinkButton

  • New optional outlined prop: MUI variant="outlined", same external (https) vs internal behaviour as before (<a target="_blank"> vs Next Link).
  • Typography for outlined and contained variants is driven by theme.typography instead of hard-coded font rules in the component.
  • Layout details (padding, min-height for outlined) stay in the component; border colour uses theme.palette.custom.outline.

theme.ts

  • Adds palette.custom.outline (#71787E, M3 sys/light/outline).
  • Adds typography variants: outlineButton, outlineButtonSmall, linkButtonContained, linkButtonContainedSmall.

Other

  • LinkButton unit tests: wrapped with ThemeProvider so theme tokens resolve.

Type

  • Bug Fix
  • New Feature
  • Code Refactor
  • Documentation
  • Other

Related Issue

Screenshots

before:
image

Now:
image

Testing

  • Unit: pnpm test -- LinkButton.test
  • Manual: Home hero — “Join our Slack” is an outlined pill; link opens Slack in a new tab.
  • Note: Full pnpm test may show unrelated failures (e.g. mentee registration test timeout).

Pull request checklist

Please check if your PR fulfills the following requirements:

  • I checked and followed the contributor guide
  • I have tested my changes locally.
  • I have added a screenshot from the website after I tested it locally

@sonarqubecloud
Copy link
Copy Markdown

sonarqubecloud bot commented Apr 5, 2026

@@ -22,4 +28,15 @@ describe('LinkButton', () => {
expect(button.closest('a')).toHaveAttribute('target', '_blank');
expect(button.closest('a')).toHaveAttribute('rel', 'noopener noreferrer');
});
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.

The existing renders an external link test still calls bare render() — it was not updated alongside the internal test. Since sx now calls theme.typography.linkButtonContained, running without ThemeProvider means those custom variants resolve to undefined. Tests still pass (no style assertions), but this is inconsistent with the fix applied to the internal test.

-    render(<LinkButton href="https://external.com">External Link</LinkButton>);
+    renderWithTheme(<LinkButton href="https://external.com">External Link</LinkButton>);

expect(link).toBeInTheDocument();
expect(link.closest('a')).toHaveAttribute('href', 'https://external.com');
});
});
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.

The outlined branch has two code paths — external (<a>) and internal (Next.js Link) — but only the external path is tested. A companion test would complete the coverage:

it("renders an outlined internal link", () => {
  renderWithTheme(
    <LinkButton href="/internal" outlined>
      Outlined Internal
    </LinkButton>,
  );
  const link = screen.getByRole("link", { name: /outlined internal/i });
  expect(link).toBeInTheDocument();
  expect(link.closest("a")).toHaveAttribute("href", "/internal");
});

Copy link
Copy Markdown
Contributor

@womencodingcommunity womencodingcommunity left a comment

Choose a reason for hiding this comment

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

Great contribution! The outlined prop is well-structured, the theme token centralisation is clean, and wrapping tests with ThemeProvider was the right fix. The two inline notes are optional follow-ups (wrap the external link test in renderWithTheme for consistency, and add a test for the internal outlined path) — neither blocks merge.

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.

2 participants