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: Set Theme Variant and Layout defaults in config #147

Closed
wants to merge 5 commits into from

Conversation

taigrr
Copy link
Contributor

@taigrr taigrr commented Oct 26, 2020

Description

Allows default configuration of theme and layout as described in #121.

Order of precedence is as follows:

  • localstorage (user's preference)
  • configuration file
  • browser preference (applies to theme only, as there's no media query for layout)
  • light theme

Fixes #121

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist:

  • I read & comply with the contributing guidelines
  • I have tested my code for new features & regressions on both mobile & desktop devices, using the latest version of major browsers.
  • I have made corresponding changes the documentation (README.md).
  • I've check my modifications for any breaking change, especially in the config.yml file

@taigrr taigrr changed the title Scheme+layout Set Theme Variant and Layout defaults in config Oct 26, 2020
@taigrr taigrr changed the title Set Theme Variant and Layout defaults in config feat: Set Theme Variant and Layout defaults in config Oct 26, 2020
@bastienwirtz
Copy link
Owner

thanks for the PR @taigrr! I'll look into it and test it asap.

Copy link
Owner

@bastienwirtz bastienwirtz 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 the great contribution @taigrr! A few notes, but it looks good 👍

connectivityCheck: true # whether you want to display a message when the apps are not accessible anymore (VPN disconnected for example)

# Optional theming
theme: default # 'default' or one of the theme available in 'src/assets/themes'.
theme_use_dark: false # true or false, useful for overriding browser default in new sessions
Copy link
Owner

Choose a reason for hiding this comment

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

As the related settings is named colors, I see something like that:

default_colors: dark # 'light', 'dark', or 'auto' (default, base on browser preference) 

It's also a bit more explicit (I like when the config speaks for itself, with little documentation necessary)

this.$emit("updated", this.isDark);
: this.isDark === null
? matchMedia("(prefers-color-scheme: dark)").matches
: this.isDark;
Copy link
Owner

Choose a reason for hiding this comment

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

Chaining 2 ternary operator is not the best for readability, could you replace it with if / else statements?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, I was trying to match the syntax of a similar code block already in the project. Not sure where it was but I'll update mine.

name: String,
icon: String,
iconAlt: String,
vlayout: Boolean,
Copy link
Owner

Choose a reason for hiding this comment

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

Why not keeping the generic component and add a default prop? I think you could achieve the same but I might have missed something. Idealy reusable components are better and I was thinking of using the SettingsToggle component for a couple of other options I might add at some point.

@finzzz
Copy link

finzzz commented Jul 18, 2021

@taigrr Why did you close this? I'm excited to see this feature to be implemented though...

@taigrr
Copy link
Contributor Author

taigrr commented Jul 18, 2021

@finzzz I didn't see much hope it would get merged and I don't have time to work on this project anymore, but these changes are active on my fork. If you want to use that instead or fork my fork and pr the changes back here, be my guest

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.

[Feature] Change default theme and layout from config
3 participants