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 admin panel option to define a default theme for site #269

Merged
merged 16 commits into from
Nov 18, 2023

Conversation

e-five256
Copy link
Member

@e-five256 e-five256 commented Nov 16, 2023

This adds an admin drop down which lets admins pick from the existing themes to set as a default for users who have not chosen a theme via the sidebar

image

By default, with no env or admin setting defined, this will switch people to see the tokyo night theme this will continue to use auto detection for light/dark themes, but via css rather than js

It also removes the javascript theme swapping based on system or user agent settings. I found the theme swapping, especially when javascript is slow to load, very jarring when visiting mbin instances, but it is possible there are people who find it useful (just recently I saw a post about someone who swaps between light and dark system settings throughout the day based on time). (edit: this was kept but moved to css auto detection)

There's an alternative middle ground perhaps where the JS is kept, but admins can define which theme gets used in light/dark mode, with perhaps a checkbox to just use the same theme in both, potentially.


this also moves the light theme into its own file and moves up some root vars. the root vars themselves are untouched, just moved around to better describe which ones are shared between all themes and which ones were specific to the previously default, light theme. My thinking is that we can next begin to style the root vars in a new theme, which will end up being the new default mbin theme, potentially

remove theme switching based on system settings
move light theme into folder
set tokyo night as default temporarily
move to be with other strings
simplify choice criteria and remove css prefix from var
comment out mbin theme for now
add some loc tokens
instance header seems to deal with wider fediverse settings
@e-five256 e-five256 added the enhancement New feature or request label Nov 16, 2023
nobodyatroot
nobodyatroot previously approved these changes Nov 16, 2023
Copy link
Member

@nobodyatroot nobodyatroot left a comment

Choose a reason for hiding this comment

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

Awesome! Nice having the extra flexibility with this option.

melroy89
melroy89 previously approved these changes Nov 16, 2023
derive color scheme via css rather than js
add admin options to use the auto detection, still allowing forcing the specific themes if desired
change env default back to the default, which now auto detects
remove unused active class in sidebar, outline for theme option comes from css class hierarchy
@e-five256 e-five256 dismissed stale reviews from melroy89 and nobodyatroot via 4f4448f November 17, 2023 00:01
@e-five256
Copy link
Member Author

e-five256 commented Nov 17, 2023

Sorry about the changes, wasn't sure what I wanted to do was possible, I should've probably converted to draft

@BentiGorlich brought up a desire to not see the auto detection be removed, so I believe I have maybe found a solution to all of our desires, maybe. I brought back auto detection based on user settings, it currently is only for light/dark themes and solarized light/solarized dark.

image

I moved the default back to default for the light/dark theme. this way there's no change to admins automatically (besides @nobodyatroot for running this already, sorry! (if you never saved the setting, it might swap to default, if you did save and want to swap back you always can if this gets merged)), but it still fixes the flashbang issue

So right now I'm theme "light/dark auto detect" in the admin panel with no cookie, and you can see my system setting is dark and the dark theme is highlighted as active

image

It's a bit hard to show just through images, but trust me that all I did was click "light" in my system settings, nothing else

image

the theme instantly changes to light, the light theme is shown as active. (this contrasts to how it is in main right now, where swapping system settings doesn't swap style, so this change brings it closer to github which I've realized does this because of being blinded by the light theme while working on the PR)

if I select a theme manually everything is still normal, it writes a cookie and it uses that theme above all else. the admin is also free to change to any other theme instead, even for example forcing solarized dark and ignoring the auto detection

still no flashbang issue, since it's done via css and not js the detection doesn't have to wait for the entire DOM to load

@@ -5,36 +5,36 @@
title="{{ 'kbin' }} {{ 'theme'|trans }}"
aria-label="{{ 'kbin' }} {{ 'theme'|trans }}"
rel="nofollow">
<div class="{{ html_classes('theme', 'kbin', constant('App\\Controller\\User\\ThemeSettingsController::KBIN') == theme ? 'active' : '' ) }}"></div>
Copy link
Member Author

@e-five256 e-five256 Nov 17, 2023

Choose a reason for hiding this comment

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

I mentioned in the commit but these active classes didn't seem to do anything, the outline to highlight which theme is selected comes from assets/styles/components/_sidebar.scss (that I modified to add the auto detect cases) which is solely based off css selectors like .theme--dark .theme.dark, as that is enough to tell which theme is active. There weren't any styles added by the .active as far as I could see

@e-five256
Copy link
Member Author

e-five256 commented Nov 17, 2023

It does slightly increase the app.css file size. The code doesn't have to duplicate the vars, but this was the only way I could think of to order the @media checks 🤔 from my understanding it basically duplicates the entries in each part for the compiled file. There might be a way to write it another way which works more optimally that I'm not thinking of

Edit: I think the bigger difference is sites using gzip compression or not, the difference compressed doesn't seem high at all, though I guess after it's uncompressed it does have a bit more size on local temp device storage, we're talking sub 300kb, if there's a better way I think it can be refactored but it's not a major concern of mine.

use include instead of import, the compiled css looks exactly the same, but this seemed more accurate to the desire of reusing the var lists for both auto detection and specific selection
.env.example Show resolved Hide resolved
@e-five256 e-five256 merged commit fd9c941 into main Nov 18, 2023
7 checks passed
@e-five256 e-five256 deleted the feature/default-theme branch November 18, 2023 00:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants