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

Add option to make Light/Dark mode using system setting. #4341

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

Conversation

sparshalc
Copy link
Contributor

This pull request resolves issue #4225 . The pertinent code has been modified to provide users with the flexibility to synchronize the application's light/dark mode with their system settings.

Changes Made

  • Updated the codebase to incorporate an option for users to set the light/dark mode based on their system preferences.

Screenshots
image
image
image

@KevinMulhern KevinMulhern self-requested a review January 16, 2024 13:52
@KevinMulhern KevinMulhern self-assigned this Jan 16, 2024
@KevinMulhern
Copy link
Member

Thanks @sparshalc, this is cool! I'll get it reviewed as soon as I can.

Copy link
Member

@KevinMulhern KevinMulhern left a comment

Choose a reason for hiding this comment

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

Thanks for contributing @sparshalc. It needs a bit of polish, but overall looking really good. Nice work!

I can't comment directly on the svg file so I'll do that here. We use Heroicons for any UI icons we use in the app. To keep our icons consistent, would you mind swapping out the system.svg in this PR with the computer-desktop.svg icon they have please?

app/components/theme/switcher_component.yml Outdated Show resolved Hide resolved
app/javascript/controllers/handle_theme_controller.js Outdated Show resolved Hide resolved
app/components/theme/switcher_component.html.erb Outdated Show resolved Hide resolved
app/components/theme/switcher_component.html.erb Outdated Show resolved Hide resolved
app/javascript/controllers/handle_theme_controller.js Outdated Show resolved Hide resolved
app/models/users/theme.rb Show resolved Hide resolved
@KevinMulhern KevinMulhern added the create-review-app Create a Heroku review app for pull request label Jan 30, 2024
@KevinMulhern
Copy link
Member

KevinMulhern commented Jan 30, 2024

Hey @sparshalc, thanks for making those updates. I'll get it reviewed again as soon as I can. But please make sure tests and linters are passing on CI before asking for a re-review.

@KevinMulhern KevinMulhern temporarily deployed to odin-review-app-pr-4341 January 30, 2024 08:31 Inactive
@KevinMulhern KevinMulhern temporarily deployed to odin-review-app-pr-4341 January 30, 2024 08:42 Inactive
app/components/mobile_theme_switcher_component.rb Outdated Show resolved Hide resolved
app/components/mobile_theme_switcher_component.html.erb Outdated Show resolved Hide resolved
app/components/theme/switcher_component.html.erb Outdated Show resolved Hide resolved
app/javascript/controllers/theme_switcher_controller.js Outdated Show resolved Hide resolved
app/javascript/controllers/theme_switcher_controller.js Outdated Show resolved Hide resolved
app/javascript/controllers/theme_switcher_controller.js Outdated Show resolved Hide resolved
app/javascript/controllers/theme_switcher_controller.js Outdated Show resolved Hide resolved
app/components/theme/switcher_component.html.erb Outdated Show resolved Hide resolved
app/components/theme/switcher_component.html.erb Outdated Show resolved Hide resolved
app/models/users/theme.rb Show resolved Hide resolved
@KevinMulhern KevinMulhern temporarily deployed to odin-review-app-pr-4341 February 1, 2024 11:58 Inactive
@KevinMulhern KevinMulhern temporarily deployed to odin-review-app-pr-4341 February 8, 2024 16:51 Inactive
@KevinMulhern
Copy link
Member

KevinMulhern commented Feb 18, 2024

Sorry for the delay @sparshalc, I've been meaning to get to this all week.

I'm just realising the event to keep TOP's theme in sync with the users system theme isn't working. Nothing will happen when that event triggers, it will always try to update the theme as the current theme.

We'd likely need something like this:

connect() {
    if (this.currentThemeValue === 'system') {
    this.updateTheme(this.selectedSystemTheme());
      this.addThemeChangeListener();
    }
  }

  addThemeChangeListener() {
    const updateThemeHandler = () => this.updateTheme(this.selectedSystemTheme())

    window.matchMedia('(prefers-color-scheme: dark)').addEventListener('change', updateThemeHandler);
  }

  selectedSystemTheme() {
    return window.matchMedia('(prefers-color-scheme: dark)').matches ? 'dark' : 'light';
  }

But that would mean we'd have to persist "system" in the theme cookie. Then use that to trigger the JS to set the theme to whatever the users system preferences are.

But, that has issues. Our views are server side rendered, which is fine when we know the dark or light theme before sending the HTML response back and the CSS is loaded in the browser. But if we need JS to set the user system theme after the HTML is sent, we'll get flashing:

Screen.Recording.2024-02-18.at.18.54.06.mov

Turbo is helping us a little with internal links, it won't flash on those. But anytime the user first enters the site or hard reloads with system theme selected it will flash.

I can think of a couple of ways around this, but they are all complex / messy; and definitely not worth doing for a "nice to have" feature. Do you have any ideas?

I'm starting to see why most of the examples out there for system themes are using SPA frameworks lol.

@sparshalc
Copy link
Contributor Author

Hey @KevinMulhern , what do you think about hiding the root element once an updateTheme request is sent, and then showing it again once the theme is selected?

image

We can start by applying a class called 'theme-hidden' to the root element, which sets its CSS display property to 'none'. Then, we'll remove this class using JavaScript after each theme request is sent.

theodin.webm

@KevinMulhern
Copy link
Member

KevinMulhern commented Feb 23, 2024

It's a little too dangerous and wide reaching for me. If the updateTheme method doesn't fire for any reason, whether it be a syntax error in the theme controller or an issue with the JS assets loading, the site would be unusable. Light/dark mode should never have that kind of reach.

The other option I've been reading about is using a render blocking JS script, but thats not great either.

Sorry @sparshalc, I know how much work you've put into this PR but I think this might be a show stopper. If it was a widely requested feature, we'd be much more tolerant of throwing a couple of hacks in to get this to work. But theres been no activity in the issue, its not worth living with code that will limit us in the future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
create-review-app Create a Heroku review app for pull request
Projects
Status: 👀 In review
Development

Successfully merging this pull request may close these issues.

2 participants