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

feat: add a "System" color scheme based on the user's OS light/dark mode #2281

Merged

Conversation

gboquizosanchez
Copy link
Contributor

This PR brings a new schema option based in the schema you have in your OS.

How it works

Once you select this new option and change your OS scheme. The page automatically will change.

Currently the user has to select light, dark or black as option. So, it's really annoying if your schema has been changed automatically.

For instance, if the sun fallen and your schema it's on light mode, and change, the page will keep in light mode. But with this option, it will auto detect and it will turn the page into dark mode.

@wescopeland wescopeland changed the title feat(auto-theme) Added a new event to auto detect OS scheme feat: added a new event to auto detect OS scheme Mar 8, 2024
@wescopeland
Copy link
Member

I'm running macOS Sonoma 14.2.1 and the latest version of Google Chrome. My system color scheme is light mode. When selecting "Auto", nothing seems to happen.

Screenshot 2024-03-08 at 6 55 07 PM

@gboquizosanchez
Copy link
Contributor Author

It only had detected the schema change. I have put a new initial value to detect properly current OS schema.

Take a look, and give me some feedback to this.

@wescopeland
Copy link
Member

Now when I select Dark, the UI switches to light mode and has a flash of color schema change on every refresh.

Area.mp4

@gboquizosanchez
Copy link
Contributor Author

I have pushed new changes, please check and confirm.

Sorry for the inconveniences. 😅

@wescopeland
Copy link
Member

On Auto, there is still a flash of color schema change on every page load.

Area.mp4

@gboquizosanchez
Copy link
Contributor Author

The problem here is caused when the page is rendered. Once is loaded is changed to the user preference. I'll check a possible workaround to disable this issue.

@gboquizosanchez
Copy link
Contributor Author

No flashes already. I hope everything is okay now.

@wescopeland
Copy link
Member

From a UX perspective, I'm happy with this solution. Thanks for working through it 🙂

I only have two other points of critique:

  • Minor: I suggest renaming "Auto" to "System". A label of "System" may be less ambiguous. I've seen "System" used to describe this mode-changing behavior in other apps (including GitHub).
  • The pure CSS implementation is indeed an optimal approach as we're no longer waiting for client-side execution; I'm glad this is where we ultimately landed. However, we have a new problem to solve: there's a lot of duplication between light and auto. I'm nervous this could lead to subtle bugs in the future. There must be an elegant/clever way to cut down this duplication.

@gboquizosanchez
Copy link
Contributor Author

gboquizosanchez commented Mar 10, 2024

The change to system has been made already.

As for you saying about CSS, I agree with you. But, the only solution I see is to put light variables into :root space. But that isn't cool either.

  --bg-color: rgb(26, 26, 26); /*#1a1a1a*/
...
  --divider-color: rgb(90, 90, 90);
  --light-bg-color: rgb(248, 248, 248);
  --light-box-bg-color: rgb(255, 255, 255);
  --light-box-shadow-color: rgb(22, 22, 22, .3);
  --light-embed-color:  rgb(240, 240, 240);
  --light-embed-highlight-color: rgb(230, 230, 230);
  --light-heading-color: rgb(50, 50, 50);
  --light-link-hover-color: rgb(0, 0, 0);
  --light-menu-link-color: rgb(66, 66, 66);
  --light-menu-link-hover-color: rgb(0, 0, 0);
  --light-text-color: rgb(66, 66, 66);
  --light-text-color-muted: rgb(130, 130, 130);
  --light-divider-color: rgb(190, 190, 190);
}

...

[data-scheme="light"] {
  --bg-color: var(--light-bg-color);
  --box-bg-color: var(--light-box-bg-color);
  --box-shadow-color: var(--light-box-shadow-color);
  --embed-color: var(--light-embed-color);
  --embed-highlight-color: var(--light-embed-highlight-color);
  --heading-color: var(--light-heading-color);
  --link-hover-color: var(--light-link-hover-color);
  --menu-link-color: var(--light-menu-link-color);
  --menu-link-hover-color: var(--light-menu-link-color);
  --text-color: var(--light-text-color);
  --text-color-muted: var(--light-text-color-muted);
  --divider-color: var(--light-divider-color);
}

[data-scheme="system"] {
  @media (prefers-color-scheme: light) {
    --bg-color: var(--light-bg-color);
    --box-bg-color: var(--light-box-bg-color);
    --box-shadow-color: var(--light-box-shadow-color);
    --embed-color: var(--light-embed-color);
    --embed-highlight-color: var(--light-embed-highlight-color);
    --heading-color: var(--light-heading-color);
    --link-hover-color: var(--light-link-hover-color);
    --menu-link-color: var(--light-menu-link-color);
    --menu-link-hover-color: var(--light-menu-link-color);
    --text-color: var(--light-text-color);
    --text-color-muted: var(--light-text-color-muted);
    --divider-color: var(--light-divider-color);
  }
}

If you have some idea about this I really appreaciate it.

@wescopeland
Copy link
Member

Think about how you might group color theme conditions to apply the same styles without repetition for each individual scheme. We can combine CSS selectors to target multiple specific scenarios with a single set of rules. At a minimum, each theme can be reduced to two blocks (vs the current set of three) which would resolve much of the duplication problem.

@wescopeland
Copy link
Member

I think we're almost there. While doing a final review moments ago, I noticed a minor bug/discrepancy.

Light mode
Screenshot 2024-03-11 at 4 06 12 PM

System (light)
Screenshot 2024-03-11 at 4 06 16 PM

@wescopeland wescopeland self-assigned this Mar 11, 2024
@gboquizosanchez
Copy link
Contributor Author

Fixed, I was missed table styles. Sorry.

@wescopeland
Copy link
Member

No worries, that fixed it. Thanks!

@wescopeland wescopeland changed the title feat: added a new event to auto detect OS scheme feat: add a "System" color scheme based on the user's OS light/dark mode Mar 11, 2024
@wescopeland wescopeland removed their assignment Mar 11, 2024
@wescopeland wescopeland merged commit b8ac41d into RetroAchievements:master Mar 24, 2024
5 checks passed
@wescopeland
Copy link
Member

Hi @gboquizosanchez,

Thanks for your first contribution to RAWeb. This will go out in our next site release (date TBD).

@gboquizosanchez
Copy link
Contributor Author

Hi @wescopeland

Thank you for the review and all the comments. I’ll be happy to assist you, guys, if you need me.

@gboquizosanchez gboquizosanchez deleted the feature/auto-theme branch March 24, 2024 19:07
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