Skip to content

feat(ui): Add dark/light theme toggling sitewide #985

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 4 commits into from
Closed

feat(ui): Add dark/light theme toggling sitewide #985

wants to merge 4 commits into from

Conversation

davis9001
Copy link

@davis9001 davis9001 commented Feb 23, 2025 •
edited
Loading

This PR adds dark/light theme to the frontend by adding the island: <ThemeToggle /> and a script in the <head> tag to avoid FOUC (Flash of Unstyled Content).

Closes: #18

General TODO list:

  • Add islands/ThemeToggle.tsx
  • Add <ThemeToggle /> to components/Header.tsx
  • Add <script> in <head> to _app.tsx
  • Save user selected theme in localStorage.theme on click of <ThemeToggle />
  • Checked on mobile
  • Avoids FOUC
  • Add color CSS vars to styles.css
  • Add colors to Tailwind config > theme > extend
  • Apply colors site wide:
    • /
    • Body background and text
    • Main links (header etc.)
    • Scrollbars
    • Form inputs
    • Selected text
    • Orama search bar
    • Markdown body
    • pre and code boxes
    • /docs
    • /new
    • /packages/*
    • /account
    • /admin
  • Pass GitHub CI tests/checks

image

image

image

image

image

@CLAassistant
Copy link

CLAassistant commented Feb 23, 2025 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@davis9001 davis9001 marked this pull request as draft February 23, 2025 17:03
@davis9001 davis9001 marked this pull request as ready for review February 23, 2025 18:15
AugustinMauroy
AugustinMauroy suggested changes Feb 24, 2025
View reviewed changes
Copy link
Contributor

@AugustinMauroy AugustinMauroy left a comment

Choose a reason for hiding this comment

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

you are doing it wrong tailwind have a dark: prefix to apply style in dark

AugustinMauroy
AugustinMauroy reviewed Feb 24, 2025
View reviewed changes
frontend/static/markdown.css
@@ -0,0 +1,799 @@
/* Override css vars for markdown and code blocks (light and dark). */
Copy link
Contributor

Choose a reason for hiding this comment

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

wow you have spend a lot of time for that. But it's a bad practice to override something

Copy link
Author

Choose a reason for hiding this comment

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

I don't see anything wrong with overriding CSS vars in this case. In an ideal world I wouldn't do that but seeing as the targets in question are external (and they provide an easy way to update the theme with CSS vars) this was IMHO the best option.

Copy link
Contributor

@AugustinMauroy AugustinMauroy Feb 25, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

So if it's external let's contribute to the dep (pr already opened by me for deno doc). You should never override your dep. If you need something from your dep:

  • Propose change if it's OSS
  • Use an other dep that do the same job
  • Write your own implementation

Copy link
Contributor

Choose a reason for hiding this comment

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

also I just verify and it's already supported by dep
https://github.com/denoland/deno-gfm?tab=readme-ov-file#styling

@AugustinMauroy
Copy link
Contributor

also on provided screenshot, the contrast ratio is horrible

@davis9001 davis9001 changed the title Add dark/light theme toggling sitewide feat(ui): Add dark/light theme toggling sitewide Feb 25, 2025
@davis9001
Copy link
Author

you are doing it wrong tailwind have a dark: prefix to apply style in dark

I use dark: prefixes where it makes sense (sparingly) but I feel that it's better to not repeat that everywhere on every element, hence the use of CSS vars.

also on provided screenshot, the contrast ratio is horrible

I see some room for improvement perhaps but could you be more specific please?

@AugustinMauroy
Copy link
Contributor

AugustinMauroy commented Feb 25, 2025 •
edited
Loading

I use dark: prefixes where it makes sense (sparingly) but I feel that it's better to not repeat that everywhere on every element, hence the use of CSS vars.

Your point of view is right. But the project use tailwind. So IMO we should use tailwind approche.

I see some room for improvement perhaps but could you be more specific please?

on landing page:

  • button border
  • search bar you should do something like border-jsr-cyan-600 dark:border-jsr-cyan-400
  • all link
  • table jsr-cyan can be little bit more lighter

@EGAMAGZ
Copy link
Contributor

EGAMAGZ commented Feb 25, 2025 •
edited
Loading

I like the way you implemented dark theme, it looks beatiful :D. IMO the contrast ratio its fine

EGAMAGZ
EGAMAGZ reviewed Feb 25, 2025
View reviewed changes
Copy link
Contributor

@EGAMAGZ EGAMAGZ left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I suggest the usage of the @preact-icons/tb package to keep the icons uniform. All icons

@crowlKats
Copy link
Collaborator

I just read through this PR; I have to mostly agree with @AugustinMauroy.
denoland/deno_doc#697 is a pre-requisite for this, and shouldnt just be completely achieved in the jsr repository.
The contrast ratio on some of the pages and elements is fine, but on others it is indeed somewhat dubious.
I would like to mention that #18 is marked as "Needs Plan", and it does also say that denoland/deno_doc#697 is a prerequisite; we probably should make it clearer by making it part of contributing guideline or PR templates that issues marked as "Needs Plan" should not be worked on without prior discussion with maintainers.
I hate to waste effort, especially since it seems a decent amount of work went into this, but i will have to decline this PR.

@crowlKats crowlKats closed this Mar 11, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@EGAMAGZ EGAMAGZ EGAMAGZ left review comments

@AugustinMauroy AugustinMauroy Awaiting requested review from AugustinMauroy

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Implement a dark theme
5 participants
@davis9001 @CLAassistant @AugustinMauroy @EGAMAGZ @crowlKats