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 "automatic" dark mode option to just use browser preference without toggle button? #1509

Closed
caendesilva opened this issue Dec 25, 2023 · 4 comments · Fixed by #1697
Closed

Comments

@caendesilva
Copy link
Member

caendesilva commented Dec 25, 2023

Could be nice to have a ternary switch in the toggle-theme-button component to just use the browser preference. This would then work like in Breeze and could be nice for simple landing pages.

@caendesilva
Copy link
Member Author

caendesilva commented Feb 7, 2024

Hmm, wondering if this could be something for the features refactor in the roadmap #1499

'features' => [
    Features::darkmode(withoutButtons: true),
],

Edit: This will no longer work after the move to using a features enum

@caendesilva
Copy link
Member Author

Three possible implementation approaches:

public static function hasDarkmodeButtons(): bool
{
    return static::hasDarkmode() && Config::getBool('hyde.darkmode.buttons', true);
}
public static function hasDarkmodeButtons(): bool
{
    return static::hasDarkmode() && static::enabled(Feature::DarkmodeButtons);
}
public static function hasDarkmodeButtons(): bool
{
    return static::hasDarkmode() && static::enabled(Feature::ButtonLessDarkmode);
}

@caendesilva
Copy link
Member Author

Three possible implementation approaches:

AI Code Review

To determine the option that offers the best developer experience (DX) for implementing the "automatic" dark mode option, let's evaluate each proposal in terms of simplicity, clarity, and maintainability.

  1. Proposal 1:
public static function hasDarkmodeButtons(): bool
{
    return static::hasDarkmode() && Config::getBool('darkmode.buttons', true);
}

This proposal directly uses a config option darkmode.buttons to determine if dark mode buttons should be displayed. It's straightforward and easy to understand. However, it adds another config option, which might not be ideal if the feature is simple and doesn't need frequent toggling.

  1. Proposal 2:
public static function hasDarkmodeButtons(): bool
{
    return static::hasDarkmode() && static::enabled(Feature::DarkmodeButtons);
}

This proposal introduces a new enum case DarkmodeButtons, which is more semantically meaningful. However, it requires adding an enum case for a relatively small feature, which might be considered over-engineering.

  1. Proposal 3:
public static function hasDarkmodeButtons(): bool
{
    return static::hasDarkmode() && static::enabled(Feature::ButtonLessDarkmode);
}

This proposal is similar to Proposal 2 but uses a different enum case name. It also suffers from the same potential issue of over-engineering for a small feature.

Considering the context and the aim for simplicity and clarity, Proposal 1 might be the best option. It directly uses a config option without introducing additional complexity. However, if you foresee the need for more granular control over dark mode-related features in the future, Proposal 2 might be a better choice as it sets a precedent for using enums for feature flags. It depends on the anticipated future requirements and preferences of the development team.

@caendesilva
Copy link
Member Author

Think I will go with the first option, and having it as a "hidden" option. If we see it being used, we can create an enum case for it in the future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
1 participant