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

feat: migrate text and headings to tailwind #13386

Merged
merged 9 commits into from
Jul 16, 2024

Conversation

TylerAPfledderer
Copy link
Contributor

@TylerAPfledderer TylerAPfledderer commented Jul 12, 2024

Description

Migrates text and heading styles to Tailwind

  • Creates a fontSize set of font-size/line-height default pairs in the tailwind config.
  • Removes Text and Heading components.
    • Favors setting the heading level font-sizes in global.css
  • Adds Flex and Center stacking components locally
    • These components are in addition to the existing stacking components added in feat: extend ShadCN initialization #13347
    • To be reusable throughout the project in place of the Chakra components of the same name.

Breaking Changes

Due to the approach taken to styling the heading levels, this might create some unexpected results when removing the Heading components in prod.

For example, see instances where the Accordion is currently used. Provided an initial fix for the accordion story to revert the heading styling to initial (essentially "unsetting" the font-size and font-weight styles). When the accordion migration is addressed, will need to favor this same approach or use a CSS selector to override or ignore the declared heading level styles, then add to the styles for specific renders.

Related Issue

Continuation of #13382

Copy link

netlify bot commented Jul 12, 2024

Deploy Preview for ethereumorg ready!

Name Link
🔨 Latest commit 2fa34a7
🔍 Latest deploy log https://app.netlify.com/sites/ethereumorg/deploys/6696277820ab74000843b199
😎 Deploy Preview https://deploy-preview-13386--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: 43 (🔴 down 8 from production)
Accessibility: 92 (no change from production)
Best Practices: 89 (🔴 down 9 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.

@TylerAPfledderer TylerAPfledderer marked this pull request as ready for review July 15, 2024 02:23
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.

Great @TylerAPfledderer

Looks good from the code side. I'll double check later on the preview deploy if there's no other part affected by the breaking changes you mentioned.

@@ -141,13 +146,41 @@

@layer base {
body {
@apply bg-background font-body leading-base text-body;
@apply bg-background font-body text-body text-sm lg:text-md;
Copy link
Member

Choose a reason for hiding this comment

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

I think we can delete the global styles from Chakra now.

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!

The Accordion case that you mentioned is the only case where we are using a h2 tag and not a Heading component. So, I've tested the app and it looks like everything is working fine. We should be good.

@pettinarip pettinarip merged commit c175e35 into ethereum:dev Jul 16, 2024
9 of 10 checks passed
@TylerAPfledderer TylerAPfledderer deleted the feat/shadcn-migrate-text branch July 16, 2024 13:37
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
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants