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

refactor: button and link with stories #4820

Merged
merged 1 commit into from
Jan 19, 2024
Merged

Conversation

fbwoolf
Copy link
Contributor

@fbwoolf fbwoolf commented Jan 17, 2024

Try out this version of Leather — Extension build, Test report

This PR refactors our LeatherButton to Button and separates out a new Link component w/ recipe. I wanted to get this up for review, but I still need to QA all the changes.

TODO:

  • Look at if can replace uses of styled.button w/ shared Button
  • QA Link component use as replacement for the old Button variant='link' and 'text'
  • Add invert styling for Link component

What do people think of the Link variant names: underlined (as default) and text (when not underlined?)

Screenshot 2024-01-17 at 3 09 02 PM Screenshot 2024-01-17 at 3 09 26 PM

@fbwoolf fbwoolf marked this pull request as draft January 18, 2024 01:09
@fbwoolf fbwoolf force-pushed the refactor/button-with-stories branch 5 times, most recently from 5298cf2 to 127108e Compare January 18, 2024 02:23
Copy link
Contributor

@pete-watters pete-watters left a comment

Choose a reason for hiding this comment

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

Great work here @fbwoolf , thanks for taking this on.

I added some feedback but it LGTM 👍 let me know when it's ready for full review and I can re-check.

I encountered one other link component along the way: ExternalLink. We seem to have that and also ExternalLinkIcon. Maybe we can move these into the one Link story and do a refactor?

We seem to sometimes pass ExternalLinkIcon into InfoCardBtn which we can hopefully replace with Link / Button.

That could be a job for another task but just mentioning now lest I forget

src/app/components/edit-nonce-button.tsx Outdated Show resolved Hide resolved
theme/recipes/link-recipe.ts Outdated Show resolved Hide resolved
@fbwoolf fbwoolf force-pushed the refactor/button-with-stories branch from 127108e to 243a9c4 Compare January 18, 2024 18:42
@fbwoolf fbwoolf linked an issue Jan 18, 2024 that may be closed by this pull request
2 tasks
@fbwoolf fbwoolf force-pushed the refactor/button-with-stories branch from 243a9c4 to 1e68cd3 Compare January 18, 2024 22:23
@fbwoolf fbwoolf linked an issue Jan 19, 2024 that may be closed by this pull request
@fbwoolf fbwoolf force-pushed the refactor/button-with-stories branch 2 times, most recently from ff106e4 to 6016325 Compare January 19, 2024 02:20
@fbwoolf fbwoolf marked this pull request as ready for review January 19, 2024 02:21
@fbwoolf
Copy link
Contributor Author

fbwoolf commented Jan 19, 2024

This should be ready to review. I linked an issue here that was opened to address our use of styled.button separate from the old LeatherButton, but I'm not sure we need to do anything to replace those. 🤔 We could create an unstyled variant of the Button, but I'm not sure that is necessary?

Copy link
Contributor

@pete-watters pete-watters left a comment

Choose a reason for hiding this comment

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

Great work here @fbwoolf . It's great to get these components in and some great refactors and improvements along the way 🚀

@pete-watters
Copy link
Contributor

This should be ready to review. I linked an issue here that was opened to address our use of styled.button separate from the old LeatherButton, but I'm not sure we need to do anything to replace those. 🤔 We could create an unstyled variant of the Button, but I'm not sure that is necessary?

I added a comment to that task as I'm not sure if we need to do it / the urgency. It could be nice to use the new components for all styled.button / styled.a but then there could be other cases where things are fine as they are.

@fbwoolf fbwoolf force-pushed the refactor/button-with-stories branch from 6016325 to 771f014 Compare January 19, 2024 18:59
@fbwoolf fbwoolf force-pushed the refactor/button-with-stories branch from 771f014 to 76e72d5 Compare January 19, 2024 19:02
@fbwoolf fbwoolf added this pull request to the merge queue Jan 19, 2024
Merged via the queue into dev with commit 7d75f4a Jan 19, 2024
28 checks passed
@fbwoolf fbwoolf deleted the refactor/button-with-stories branch January 19, 2024 19:27
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.

Design System: Button (and stories)
5 participants