ethereum / ethereum-org-website

Ethereum.org is a primary online resource for the Ethereum community.
https://ethereum.org/
MIT License
5.09k stars 4.83k forks source link

Migrate EventCard to Shadcn #13966

Closed minimalsm closed 1 month ago

minimalsm commented 1 month ago

Description

Notes

  1. Hardcoded 1.6rem line height for the description text (per OldText) as the defaults we have varied too much from the current design. Our designers should pull this component in line with current DS.

  2. Slight differences in spacing compared to the original design (which had kind of unusual spacing). IMO, these are improvements but defer to @nloureiro

  3. I've hardcoded the background colors (bg-[#FCFCFC] dark:bg-[#272627]) in the CardHeader. Our default theme values didn't provide a good match for this use case. another area where we might want to consult with design

Related Issue

13946

netlify[bot] commented 1 month ago

Deploy Preview for ethereumorg ready!

Name Link
Latest commit ffee150028a9c6c49edcbafbc7f77e96ed72a867
Latest deploy log https://app.netlify.com/sites/ethereumorg/deploys/66f57cfd7a654f000783fe86
Deploy Preview https://deploy-preview-13966--ethereumorg.netlify.app
Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

Lighthouse
7 paths audited
Performance: 47 (🟢 up 1 from production)
Accessibility: 93 (no change from production)
Best Practices: 89 (🔴 down 9 from production)
SEO: 92 (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.

nloureiro commented 1 month ago

agree. The spacing was off. Thank you for the adjustments

comapring with Figma, the text size is bigger, but now we are bumping everything to 16 or above so let's keep it like this

Screen Shot 2024-09-25 11 17 39 PM

With this text size, we should do a 2-card line. It's too narrow for an MD page. Would it be on another PR to do this?

Screen Shot 2024-09-25 11 22 51 PM

minimalsm commented 1 month ago

Thanks @nloureiro

agree. The spacing was off. Thank you for the adjustments

comapring with Figma, the text size is bigger, but now we are bumping everything to 16 or above so let's keep it like this

Okay can update to 16px fontsize. With that we can probably normalise line height too?

With this text size, we should do a 2-card line. It's too narrow for an MD page. Would it be on another PR to do this?

We can handle that in #13967, which is the upcoming events grid wrapper

nloureiro commented 1 month ago

Thanks @nloureiro

agree. The spacing was off. Thank you for the adjustments comapring with Figma, the text size is bigger, but now we are bumping everything to 16 or above so let's keep it like this

Okay can update to 16px fontsize. With that we can probably normalise line height too?

yes, makes sense

With this text size, we should do a 2-card line. It's too narrow for an MD page. Would it be on another PR to do this?

We can handle that in #13967, which is the upcoming events grid wrapper

ok, got it

minimalsm commented 1 month ago

Updated font size implementation after discussion with @nloureiro:

Mobile: 16px font size Desktop: Retaining 14px font size

Rationale:

Changes implemented && ready for review :)