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 theme selector button to footer #27576

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

silverwind
Copy link
Member

@silverwind silverwind commented Oct 11, 2023

Add this button to the footer to quickly switch themes:

Screenshot 2023-10-11 at 09 04 33

Custom themes currently render with this icon, but I guess we could add some heuristics to detect dark/light/auto for theme if they have such a suffix in the name.

Maybe we want to "beautify" these names, e.g. gitea-dark to Gitea Dark or just Dark.

Also, I cleaned up various code related to theme selection.

@pull-request-size pull-request-size bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Oct 11, 2023
@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Oct 11, 2023
@silverwind silverwind added the type/feature Completely new functionality. Can only be merged if feature freeze is not active. label Oct 11, 2023
web_src/css/home.css Show resolved Hide resolved
web_src/js/features/common-global.js Show resolved Hide resolved
{{$currentTheme := ThemeName .SignedUser}}
<span class="flex-text-inline">{{svg (ThemeIcon $currentTheme) 14}} {{$currentTheme}}</span>
<div class="menu theme-menu">
{{range Themes}}
Copy link
Member

Choose a reason for hiding this comment

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

If we do that, I think it might also be a good idea to sort the themes alphabetically beforehand (once, on Gitea startup).
Otherwise, we might confuse users if the order changes because the admin changed the value of the setting.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, sorting is a good idea, can add. Not sure if really warranted to store in-memory because sorting a small array is fast, but can do.

Copy link
Member

Choose a reason for hiding this comment

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

I mean, we store the array already, so I don't see why it would consume extra memory?

Copy link
Member Author

Choose a reason for hiding this comment

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

Right, we can just sort setting.UI.Themes after config parsing, so no additional memory needed.

@silverwind
Copy link
Member Author

silverwind commented Oct 11, 2023

Regarding "nice theme name" and icons, I was thinking we could accept a alternate value for THEMES which holds a json object, so instead of

THEMES = gitea-auto,gitea-light,gitea-dark

It would also accept

THEMES = '[{"name":"gitea-auto", "title": "Auto", "icon": "gitea-eclipse"},{"name": "gitea-light", "title": "Light", "icon": "octicon-sun"},{"name":"gitea-dark", "title": "Dark", "icon": "octicon-moon"}]'

A bit unusual for our configs, but this would offer maximum flexibility to both us and the users.

Comment on lines +142 to +150
if theme == "gitea-dark" {
return "octicon-moon"
} else if theme == "gitea-light" {
return "octicon-sun"
} else if theme == "gitea-auto" {
return "gitea-eclipse"
} else {
return "octicon-paintbrush"
}
Copy link
Contributor

@hiifong hiifong Oct 15, 2023

Choose a reason for hiding this comment

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

Suggested change
if theme == "gitea-dark" {
return "octicon-moon"
} else if theme == "gitea-light" {
return "octicon-sun"
} else if theme == "gitea-auto" {
return "gitea-eclipse"
} else {
return "octicon-paintbrush"
}
switch theme {
case "gitea-dark":
return "octicon-moon"
case "gitea-light":
return "octicon-sun"
case "gitea-auto":
return "gitea-eclips"
default:
return "octicon-paintbrush"
}

Copy link
Contributor

Choose a reason for hiding this comment

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

I like the switch way of writing, but gitea doesn't seem to have a normalized way of writing this, so it's the same with either one of them.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think I will go with the JSON config, making this switch unnecessary.

Copy link
Contributor

Choose a reason for hiding this comment

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

-> Improve theme display #30671

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah that seems nice, waiting for that PR here.

@silverwind silverwind marked this pull request as draft October 17, 2023 14:40
@silverwind
Copy link
Member Author

Will pick this up again soon.

@lafriks
Copy link
Member

lafriks commented Mar 2, 2024

Maybe for built-in themes names could come from locale files so that they could be translated, for custom just use theme name

@silverwind
Copy link
Member Author

Maybe for built-in themes names could come from locale files so that they could be translated, for custom just use theme name

Would only be possible for the default one, unless we extend that config json with translation keys, but I think no one is going to bother to translate their theme names. At some point the translation madness has to stop 😆.

@lafriks
Copy link
Member

lafriks commented Mar 14, 2024

Would only be possible for the default one, unless we extend that config json with translation keys, but I think no one is going to bother to translate their theme names. At some point the translation madness has to stop 😆.

Yes, I meant only to have translatable Auto, Light, Dark theme names

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. type/feature Completely new functionality. Can only be merged if feature freeze is not active.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants