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

Shadcn migration - HubHero #13691

Merged
merged 10 commits into from
Sep 4, 2024

Conversation

saurabhburade
Copy link
Contributor

Description

Migrate HubHero to tailwind

Copy link

netlify bot commented Aug 20, 2024

Deploy Preview for ethereumorg ready!

Name Link
🔨 Latest commit 652729a
🔍 Latest deploy log https://app.netlify.com/sites/ethereumorg/deploys/66d895f7b20ebd0008d35ad1
😎 Deploy Preview https://deploy-preview-13691--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: 42 (🔴 down 8 from production)
Accessibility: 94 (🟢 up 2 from production)
Best Practices: 83 (🔴 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.

@saurabhburade
Copy link
Contributor Author

As of now using Stack from ui/flex needs explicit spacings

  • Use space-y as considering it to be the column
  • Use space-x as considering it to be the row

Container widths are differently defined than default tw widths

  • Used in chakra --eth-sizes-container-md: 768px
  • Tailwind max-w-3xl : 768px

@TylerAPfledderer
Copy link
Contributor

TylerAPfledderer commented Aug 20, 2024

As of now using Stack from ui/flex needs explicit spacings

  • Use space-y as considering it to be the column
  • Use space-x as considering it to be the row

Container widths are differently defined than default tw widths

  • Used in chakra --eth-sizes-container-md: 768px
  • Tailwind max-w-3xl : 768px

Hi @saurabhburade! Can you clarify why we need to explicitly define the spacing in this way?

Note that those utility classes use margin instead of the gap properties. And the Stack and VStack components are for vertically-aligned flex boxes, and the HStack for horizontally stacked boxes.

We will need to define how this should be handled for consistency with other contributors. I would think that if we are using flexbox, that we should favor the gap property.

@saurabhburade
Copy link
Contributor Author

saurabhburade commented Aug 21, 2024

We will need to define how this should be handled for consistency with other contributors. I would think that if we are using flexbox, that we should favor the gap property.

Yeah, this seems like the spacing prop treated as a gap property in Chakra Stack
https://github.com/chakra-ui/chakra-ui/blob/%40chakra-ui/react%402.8.0/packages/components/layout/src/stack/stack.tsx#L152

@saurabhburade
Copy link
Contributor Author

saurabhburade commented Aug 21, 2024

@pettinarip @TylerAPfledderer

  • We need to add container size classes, as of now we are using default width sizes from tailwind.
   --eth-sizes-container-sm: 640px;
   --eth-sizes-container-md: 768px;
   --eth-sizes-container-lg: 1024px;
   --eth-sizes-container-xl: 1280px;
   --eth-sizes-container-2xl: 1536px;

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.

@saurabhburade this looks great! left a couple of comments to migrate a few more things.

Regarding the container sizes, we have defined them here and they match the breakpoints we had in chakra theming.

src/components/Hero/HubHero/index.tsx Outdated Show resolved Hide resolved
src/components/Hero/HubHero/index.tsx Outdated Show resolved Hide resolved
src/components/Hero/HubHero/index.tsx Show resolved Hide resolved
src/components/Hero/HubHero/index.tsx Outdated Show resolved Hide resolved
@TylerAPfledderer
Copy link
Contributor

We will need to define how this should be handled for consistency with other contributors. I would think that if we are using flexbox, that we should favor the gap property.

Yeah, this seems like the spacing prop treated as a gap property in Chakra Stack https://github.com/chakra-ui/chakra-ui/blob/%40chakra-ui/react%402.8.0/packages/components/layout/src/stack/stack.tsx#L152

Right, it's an alias prop for gap.

Chakra v3 no longer has that prop btw, as they didn't find it semantically useful for users. So gap is the preferred property to use

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.

Nice job @saurabhburade

@pettinarip pettinarip merged commit 67eebaa into ethereum:dev Sep 4, 2024
10 checks passed
This was referenced Sep 18, 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.

3 participants