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: migrate buttons to tailwind #13419

Merged
merged 8 commits into from
Jul 23, 2024

Conversation

TylerAPfledderer
Copy link
Contributor

@TylerAPfledderer TylerAPfledderer commented Jul 18, 2024

Description

Updates the button components to use ShadCN/Tailwind styling.

Also rebuilds ButtonTwoLines to be a little more verbose in determining which type of button component it renders via a deterministic prop, to provide better type guarding.

These updates here are isolated to the tailwind directory and are not migrated to prod. Update stories are also isolated here to not effect Chromatic snapshots. (To be handled later in conjunction with syncing with updated link components)

@github-actions github-actions bot added the tooling 🔧 Changes related to tooling of the project label Jul 18, 2024
Copy link

netlify bot commented Jul 18, 2024

Deploy Preview for ethereumorg ready!

Name Link
🔨 Latest commit 81b2a45
🔍 Latest deploy log https://app.netlify.com/sites/ethereumorg/deploys/669f22fa756f2f000837e9b2
😎 Deploy Preview https://deploy-preview-13419--ethereumorg.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.
Lighthouse
Lighthouse
7 paths audited
Performance: 46 (🔴 down 5 from production)
Accessibility: 92 (no change from production)
Best Practices: 88 (🔴 down 10 from production)
SEO: 93 (no change from production)
PWA: -
View the detailed breakdown and full score reports

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link
Member

@pettinarip pettinarip left a comment

Choose a reason for hiding this comment

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

Looking great @TylerAPfledderer.

Since that this is a new Button component, we could place it under the /tailwind folder to clearly see that this component hasn't been migrated yet on the codebase like the Accordion example or the Link component.

Another thing, naming, the shadcn cli installs the components using snake-case. Do you know if we can configure it to install using CapitalizedCase? I think we should be consistent with that.

src/components/ui/buttons/Button.tsx Outdated Show resolved Hide resolved
<BaseLink
ref={ref}
activeStyle={{}}
// TODO: Redress this override when migrating the link component
Copy link
Member

Choose a reason for hiding this comment

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

Cool, yes. Once we merge this and the link PR, we can fix this.

@TylerAPfledderer
Copy link
Contributor Author

Looking great @TylerAPfledderer.

Since that this is a new Button component, we could place it under the /tailwind folder to clearly see that this component hasn't been migrated yet on the codebase like the Accordion example or the Link component.

Another thing, naming, the shadcn cli installs the components using snake-case. Do you know if we can configure it to install using CapitalizedCase? I think we should be consistent with that.

As to moving this under tailwind folder, I would be moving the updated stories in there as well so Chromatic doesn't see them. I don't think the existing story files should be replaced yet because of ButtonLink, and Button and Link I think are to coupled to check baselines before properly addressing them together.

With the component naming, I don't see an option in the schema to automatically change them. This seems to be an opinionated part of the tooling.

Also, there are two different styles of component writing? I cannot tell what the difference is!

@pettinarip
Copy link
Member

As to moving this under tailwind folder, I would be moving the updated stories in there as well so Chromatic doesn't see them. I don't think the existing story files should be replaced yet because of ButtonLink, and Button and Link I think are to coupled to check baselines before properly addressing them together.

Yeah, sounds good! yes, I don't think we can replace them atm. We can replace them later and check for any regressions that we don't currently see now.

With the component naming, I don't see an option in the schema to automatically change them. This seems to be an opinionated part of the tooling.

Right, weird that they didn't add that to the CLI. We could go later and rename them to be consistent.

Also, there are two different styles of component writing? I cannot tell what the difference is!

Me neither haha. All I see is that they use different font sizes in some places but they are pretty similar.

@TylerAPfledderer TylerAPfledderer marked this pull request as ready for review July 19, 2024 20:07
@TylerAPfledderer
Copy link
Contributor Author

@pettinarip should be good to look over. Added a comment in the UI Tests check.

@pettinarip
Copy link
Member

pettinarip commented Jul 22, 2024

@pettinarip should be good to look over. Added a comment in the UI Tests check.

Hey, I don't see this diff in the last build. Wondering if you fix it.

Edit: ah ok, the snapshot went well but when you enter SB you can see the incorrect color

Copy link
Member

@pettinarip pettinarip left a comment

Choose a reason for hiding this comment

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

@TylerAPfledderer left a few minor comments to polish a bit more but overall looks great.

tailwind/ui/buttons/Button.tsx Outdated Show resolved Hide resolved
tailwind/ui/buttons/Button.tsx Show resolved Hide resolved
tailwind/ui/buttons/Button.tsx Show resolved Hide resolved
tailwind/ui/buttons/ButtonTwoLines.tsx Outdated Show resolved Hide resolved
tailwind/ui/buttons/ButtonTwoLines.tsx Show resolved Hide resolved
Copy link
Member

@pettinarip pettinarip left a comment

Choose a reason for hiding this comment

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

LGTM!

@pettinarip pettinarip merged commit 08a4bcb into ethereum:dev Jul 23, 2024
9 of 10 checks passed
@TylerAPfledderer TylerAPfledderer deleted the feat/shadcn-migrate-button branch July 23, 2024 13:39
This was referenced Jul 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tooling 🔧 Changes related to tooling of the project
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants