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

Dark mode toggle and fixes #356

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

Dark mode toggle and fixes #356

wants to merge 6 commits into from

Conversation

jaakkonakaza
Copy link
Contributor

No description provided.

@jaakkonakaza jaakkonakaza marked this pull request as draft May 19, 2024 20:41
@jaakkonakaza jaakkonakaza marked this pull request as ready for review May 22, 2024 14:01
Copy link
Collaborator

@MikaelSiidorow MikaelSiidorow left a comment

Choose a reason for hiding this comment

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

Small quick fixes:

  • board and magazine cards still have light-themed headers
  • board and committee member placeholder image is black on black
  • mobile nav theme toggle has odd active styles (light flash)
  • mobile nav sublinks in collapsible sections are black on black
  • mobile nav external links have black borders
  • landing page event cards don't have backgrounds, so the gradient is seeping through

<Button
variant="ghost"
onClick={() => {
setTheme(theme === "dark" ? "light" : "dark");
Copy link
Collaborator

Choose a reason for hiding this comment

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

Instead of a pure toggle, what if this was a menu with "System" / "Light" / "Dark" options?

/* Shadow TOP */
radial-gradient(
farthest-side at 50% 0,
theme("colors.stone.400 / 30%"),
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we should use the brand / gray palette that the light mode site also uses, or create a new custom palette for dark mode.

The stone color palette looks nice, but it's quite different from the light theme colors

@@ -70,7 +70,7 @@ export default async function Home({
{announcement ? <AnnouncementCard news={announcement} /> : null}
</div>
<section className="space-y-4">
<h1 className="font-mono text-4xl font-bold text-gray-900">
<h1 className="dark:text-dark-heading font-mono text-4xl font-bold text-gray-900">
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is a bigger note and imo not in scope of this PR, but we could move toward replacing straight color tokens like gray-900 with more semantic tokens overall like primary-text or similar.

Would also make it easier to assign those tokens in the tailwind theme and avoid having to define both dark: and light colors separately, since primary-text could already have both variants.

Inspired by shadcn

Copy link
Collaborator

Choose a reason for hiding this comment

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

This would also make the dark mode (or light mode, whichever is not the default) less error prone

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.

2 participants