Skip to content

Conversation

@hellendag
Copy link
Member

@hellendag hellendag commented Nov 13, 2025

Summary & Motivation

Refactor ButtonLink to use CSS module instead of styled-components.

How I Tested These Changes

TS, lint, storybook.

Copy link
Member Author

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more

This stack of pull requests is managed by Graphite. Learn more about stacking.

@hellendag hellendag requested a review from bengotow November 13, 2025 22:18
@hellendag hellendag marked this pull request as ready for review November 13, 2025 22:18
...rest
}: Props) => {
const colors = color ? getColorVars(color) : {};
const textDecoration = underline ? getTextDecorationClassName(underline) : undefined;
Copy link
Contributor

Choose a reason for hiding this comment

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

The original implementation defaulted to 'hover' when underline was not provided, but this implementation only applies a class when underline is defined. To maintain the same behavior, consider changing to:

const textDecoration = getTextDecorationClassName(underline || 'hover');

This ensures the hover underline effect remains the default behavior, matching the previous implementation.

Suggested change
const textDecoration = underline ? getTextDecorationClassName(underline) : undefined;
const textDecoration = getTextDecorationClassName(underline || 'hover');

Spotted by Graphite Agent

Fix in Graphite


Is this helpful? React 👍 or 👎 to let us know.

Comment on lines +2 to +4
--button-link-color: var(--color-link-default);
--button-link-hover-color: var(--color-link-default);
--button-link-active-color: var(--color-link-default);
Copy link
Contributor

Choose a reason for hiding this comment

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

The default values for hover and active colors should be updated to match the original behavior. Currently, all three CSS variables are set to var(--color-link-default), but the hover and active states should use their respective color values from the Colors utility. Specifically, --button-link-hover-color should be set to var(--color-link-hover) to maintain consistency with the previous implementation.

Suggested change
--button-link-color: var(--color-link-default);
--button-link-hover-color: var(--color-link-default);
--button-link-active-color: var(--color-link-default);
--button-link-color: var(--color-link-default);
--button-link-hover-color: var(--color-link-hover);
--button-link-active-color: var(--color-link-active);

Spotted by Graphite Agent

Fix in Graphite


Is this helpful? React 👍 or 👎 to let us know.

@github-actions
Copy link

github-actions bot commented Nov 13, 2025

Deploy preview for dagit-storybook ready!

✅ Preview
https://dagit-storybook-42mfd2z7q-elementl.vercel.app
https://dish-button-link.components-storybook.dagster-docs.io

Built with commit 9fee99d.
This pull request is being automatically deployed with vercel-action

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