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

[UXIT-2039] Upgrade to Tailwind V4 [skip percy] #1077

Open
wants to merge 15 commits into
base: main
Choose a base branch
from

Conversation

mirhamasala
Copy link
Collaborator

@mirhamasala mirhamasala commented Feb 6, 2025

πŸ“ Description

  • Type: Upgrade

This PR upgrades the Tailwind configuration and aligns the project's styling approach with TailwindCSS 4 best practices.

πŸ› οΈ Key Changes

  • Moved all configuration from tailwind.config.js to globals.css.
  • Replaced theme extensions with native CSS classes in globals.css.
  • CSS Modules Migration:
    • Migrated text-link and toc-header styles to globals.css, following Tailwind's recommendation against using CSS modules.
    • Made tooltip.module.css and table.module.css to plain css, following Tailwind's recommendation against using CSS modules.
  • Removed SASS, as Tailwind no longer recommends using preprocessors like SASS (reference).
  • Typography Plugin:
    • Moved typography overrides to globals.css under @utility prose, based on this guidance.

πŸ”– Resources

__

Note

Copy link

Copy link

vercel bot commented Feb 6, 2025

The latest updates on your projects. Learn more about Vercel for Git β†—οΈŽ

Name Status Preview Comments Updated (UTC)
ffdweb-site βœ… Ready (Inspect) Visit Preview πŸ’¬ Add feedback Feb 11, 2025 4:24pm
filecoin-foundation-site ❌ Failed (Inspect) Feb 11, 2025 4:24pm

@barbaraperic
Copy link
Collaborator

Haven't checked the code yet, but I've noticed 2 UI discrepancies

  1. bg-brand-700 seems to be off? (card background)

Screenshot 2025-02-10 at 10 04 09

  1. Column is not sticky in the allocators table
CleanShot.2025-02-10.at.10.13.05.mp4

@barbaraperic
Copy link
Collaborator

barbaraperic commented Feb 10, 2025

Also, I don't know if it's on my side, but the laptop view seems to be broken on Safari (16.1 version) - it's showing mobile layout on every screen size?

CleanShot 2025-02-10 at 11.35.19.mp4.zip

Comment on lines 117 to 104
@layer components {
.text-link {
@apply text-brand-300;

&:hover {
@apply underline;
}

&:focus {
@apply brand-outline;
}
}
Copy link
Collaborator

@barbaraperic barbaraperic Feb 10, 2025

Choose a reason for hiding this comment

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

Tailwind utility prose seems to be overwriting the text-link class?

Screenshot 2025-02-10 at 11 57 33

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

As discussed earlier, not sure how to override this differently at the moment.

Copy link
Collaborator

Choose a reason for hiding this comment

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

We could potentially use ! - I know we generally want to avoid !important, but in this case we want to overwrite an imported library so maybe it's justified? In that case we could get rid of --tw-prose-links and have 1 class responsible for all links. Just an idea.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe adding not-prose to our custom link?

// MarkdownLink in apps/site/src/app/_components/MarkdownContent.tsx

return (
  <SmartTextLink href={href} className="not-prose">
    {children}
  </SmartTextLink>
)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We could potentially use ! - I know we generally want to avoid !important, but in this case we want to overwrite an imported library so maybe it's justified? In that case we could get rid of --tw-prose-links and have 1 class responsible for all links. Just an idea.

Where would you add this? @barbaraperic

Copy link
Collaborator Author

@mirhamasala mirhamasala Feb 11, 2025

Choose a reason for hiding this comment

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

@CharlyMartin - That works. It's a bit odd to add not-prose to something that is clearly prose but, at least, it's clear where it's coming from?

Copy link
Collaborator Author

@mirhamasala mirhamasala Feb 11, 2025

Choose a reason for hiding this comment

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

Done - e25b269 - What do you both think?

Copy link
Collaborator

@CharlyMartin CharlyMartin Feb 11, 2025

Choose a reason for hiding this comment

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

Works for me! (It's a bit odd indeed but it's clear at least yes)

startDate: CalendarEventType['start']['dateTime']
}

export function Calendar({ startDate }: CalendarProps) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Sorry I seemed to missed this, we are temporarily not using this component? πŸ€”

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes - We have removed the Governance Community Call section per #1080

Copy link
Collaborator

Choose a reason for hiding this comment

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

Gotcha, thank you! Should we remove the component? Or the calls are planning to return?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The component was already removed - So I'm guessing this is a conflict resolution issue. Thanks for catching.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done!

Copy link
Collaborator

@CharlyMartin CharlyMartin left a comment

Choose a reason for hiding this comment

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

Phew, big update! Thanks Mirha and Barbara for taking care of this!

I've left comments about things that I found confusing, not all of them may be relevant.

I'll have a second look tomorrow :)

apps/site/src/app/_components/Table/Table.css Outdated Show resolved Hide resolved
apps/site/src/app/_components/Table/Table.css Outdated Show resolved Hide resolved
apps/site/src/app/_components/Table/Table.css Outdated Show resolved Hide resolved
apps/site/src/app/_components/SearchInput.tsx Show resolved Hide resolved
apps/site/.prettierrc.json Outdated Show resolved Hide resolved
apps/site/src/app/_styles/globals.css Outdated Show resolved Hide resolved
apps/site/src/app/_styles/globals.css Show resolved Hide resolved
@mirhamasala mirhamasala changed the title [UXIT-2039] Upgrade to Tailwind V4 [UXIT-2039] Upgrade to Tailwind V4 [skip percy] Feb 11, 2025
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