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.

Sign up for GitHub

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

Jump to bottom

Create LinkButton component #5298

Merged
merged 22 commits into from
Dec 19, 2024
Merged

Create LinkButton component #5298

merged 22 commits into from
Dec 19, 2024

Conversation

mcwinter07
Copy link
Contributor

Why

This is the counterpart to the Button v3 component and closes the cap on feature parity for the original v1 button.

What

  • Adds a LinkButton component
  • Adds docs, tests, and stickersheets for visual regression
  • Updates Button docs references to Links

@changeset-bot changeset-bot
Copy link

changeset-bot bot commented Nov 21, 2024 •
edited
Loading

🦋 Changeset detected

Latest commit: 2538c7a

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@kaizen/components Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@mcwinter07
Copy link
Contributor Author

Moving this to ready for review and back into draft for the branch preview - no need to review just yet

@mcwinter07 mcwinter07 marked this pull request as ready for review November 21, 2024 04:26
@mcwinter07 mcwinter07 requested a review from a team as a code owner November 21, 2024 04:26
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Nov 21, 2024 •
edited
Loading

✨ Here is your branch preview! ✨

Last updated for commit 2538c7a: Merge branch 'main' into KZN-2594/build-link-button

@mcwinter07 mcwinter07 marked this pull request as draft November 21, 2024 04:39
@mcwinter07 mcwinter07 force-pushed the KZN-2594/build-link-button branch 2 times, most recently from 32cc077 to fcfc107 Compare November 26, 2024 05:51
@mcwinter07 mcwinter07 marked this pull request as ready for review November 26, 2024 05:51
@mcwinter07
Copy link
Contributor Author

Posing a question / addition to the API for anyone reviewing this. A common pattern I observed when I was looking at Next.js repos that consumed the Next.JS Link was that folks would pass in a Node urlObject. Currently this implementation only accepts string as a href, so wondering whether folks think it should also accept that as a type?

HeartSquared
HeartSquared reviewed Dec 3, 2024
View reviewed changes
packages/components/src/__actions__/Button/v3/Button.tsx Outdated Show resolved Hide resolved
packages/components/src/__actions__/Button/v3/_docs/Button--api-specification.mdx Outdated Show resolved Hide resolved
packages/components/src/__actions__/Button/v3/_docs/Button--api-specification.mdx Outdated Show resolved Hide resolved
packages/components/src/__actions__/Button/v3/_docs/Button--usage-guidelines.mdx Outdated Show resolved Hide resolved
packages/components/src/__actions__/LinkButton/v3/LinkButton.tsx Outdated Show resolved Hide resolved
packages/components/src/__actions__/LinkButton/v3/_docs/LinkButton--api-specification.mdx Outdated Show resolved Hide resolved
packages/components/src/__actions__/LinkButton/v3/_docs/LinkButton--api-specification.mdx Outdated Show resolved Hide resolved
packages/components/src/__actions__/LinkButton/v3/_docs/LinkButton.doc.stories.tsx Outdated Show resolved Hide resolved
packages/components/src/__actions__/LinkButton/v3/_docs/LinkButton--api-specification.mdx Outdated Show resolved Hide resolved
packages/components/src/__actions__/LinkButton/v3/_docs/LinkButton--api-specification.mdx Outdated Show resolved Hide resolved
@mcwinter07 mcwinter07 force-pushed the KZN-2594/build-link-button branch 2 times, most recently from 6fce372 to 0a0f190 Compare December 10, 2024 22:50
HeartSquared
HeartSquared reviewed Dec 17, 2024
View reviewed changes
packages/components/src/__actions__/LinkButton/v3/_docs/LinkButton--api-specification.mdx Outdated Show resolved Hide resolved
packages/components/src/__actions__/LinkButton/v3/_docs/LinkButton.doc.stories.tsx Outdated Show resolved Hide resolved
packages/components/src/__actions__/LinkButton/v3/_docs/LinkButton.spec.stories.tsx Outdated
play: async ({ canvasElement }) => {
const canvas = within(canvasElement.parentElement!)
const linkButton = canvas.getByRole("link")
await linkButton.focus()
Copy link
Contributor

Choose a reason for hiding this comment

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

It's actually exactly what you're expecting though right? The alternative is to just delete the story. You're right that you're essentially testing RAC functionality, which shouldn't be necessary.

@mcwinter07 mcwinter07 force-pushed the KZN-2594/build-link-button branch from c2c751c to d788a4f Compare December 17, 2024 23:22
@mcwinter07 mcwinter07 force-pushed the KZN-2594/build-link-button branch from d788a4f to adf956e Compare December 18, 2024 04:39
HeartSquared
HeartSquared reviewed Dec 18, 2024
View reviewed changes
packages/components/src/index.ts Outdated
Comment on lines 64 to 65
export * from './Menu'
export * from './Button'
Copy link
Contributor

Choose a reason for hiding this comment

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

Whoops omg I didn't realise this auto-added itself when I moved things around. Could I trouble you to remove this for me? Since we're currently double exporting 😅 🙏🏻

Copy link
Contributor Author

Choose a reason for hiding this comment

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

heheh nice, will axe it :)

@mcwinter07 mcwinter07 force-pushed the KZN-2594/build-link-button branch from 17b5304 to 1470490 Compare December 19, 2024 01:23
HeartSquared
HeartSquared approved these changes Dec 19, 2024
View reviewed changes
Copy link
Contributor

@HeartSquared HeartSquared left a comment

Choose a reason for hiding this comment

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

🚀

@mcwinter07 mcwinter07 merged commit 1f9edc2 into main Dec 19, 2024
19 checks passed
@mcwinter07 mcwinter07 deleted the KZN-2594/build-link-button branch December 19, 2024 04:06
@github-actions github-actions bot mentioned this pull request Dec 19, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@HeartSquared HeartSquared HeartSquared approved these changes

@dougmacknz dougmacknz Awaiting requested review from dougmacknz

@Zystix Zystix Awaiting requested review from Zystix

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@mcwinter07 @HeartSquared