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 separate dark theme color settings #12613

Merged

Conversation

meel-hd
Copy link
Contributor

@meel-hd meel-hd commented Sep 27, 2024

Proposed changes

  • Modifed the form to have two color fields (one for dark and one for light theme).
  • The colors are stored in this form"#ffffff,#000000". Where the first one is for light theme and the other for dark theme.
  • Then the custom.css consumes the values and performs a split(defined in custom_filters.py) on the color string to get the two colors.
  • Those colors then are defined in css variables and consumed by the elements that need them.
  • The css variables change base on the current theme (for dark values to be applied it is expected that the body tag has data-theme="dark")

Closes: #9372

Checklist

  • Lint and unit tests pass locally with my changes.
  • I have added tests that prove my fix is effective or that my feature works.
  • I have added documentation to describe my feature.
  • I have squashed my commits into logic units.
  • I have described the changes in the commit messages.

Other information

Preview

Dark theme selected

Screenshot 2024-09-27 125432

Light theme selected

Screenshot 2024-09-27 125537

- Modifed the form to have two color fields (one for dark and one for light theme).
- The colors are stored  in this form"#ffffff,#000000". Where the first one is for light theme and the other for dark theme.
- Then the custom.css consumes the values and performs a split(defined in custom_filters.py) on the color string to get the two colors.
- Those colors then are defined in css variables and consumed by the elements that need them.
- The css variables change base on the current theme (for dark values to be applied it is expected that the body tag has `data-theme="dark"`)
@meel-hd meel-hd marked this pull request as draft September 27, 2024 12:21
@meel-hd
Copy link
Contributor Author

meel-hd commented Sep 27, 2024

@nijel I am still working on this. I should add the data-theme attr to the body tag based on the current theme. However, I would love some early feedback

@meel-hd meel-hd marked this pull request as ready for review September 28, 2024 10:28
@meel-hd meel-hd requested a review from nijel September 28, 2024 10:29
Copy link
Member

@nijel nijel left a comment

Choose a reason for hiding this comment

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

What is missing is a migration path. When user currently has customized theme, he will get all black dark theme:

obrazek

Probably the best approach would be to do a database migration on upgrade to fix the affected settings.

@nijel nijel added this to the 5.9 milestone Oct 3, 2024
meel-hd and others added 3 commits October 4, 2024 13:27
- Insure backward compatibility with previous theme color settings.

- In the new implementation it considers if there was previous color settings. If so it applies them to dark theme colors too.
@meel-hd
Copy link
Contributor Author

meel-hd commented Oct 10, 2024

@nijel I got overwhelmed by this previous test fail it says it is missing a header_color variable in a dozen of tests. I don't know why and what to do. Any guides or thoughts would be helpful. Thanks.

@nijel
Copy link
Member

nijel commented Oct 14, 2024

I currently don't see what might be causing that.

Can we split this PR into two? One would introduce using variables in the custom CSS without doing any functional change and second would then add the dark theme customization? That way, we limit what might be causing the failures.

@meel-hd
Copy link
Contributor Author

meel-hd commented Oct 15, 2024

Thanks for the suggestion. I'll go ahead and split the PR into two as you recommended. I'll first focus on the dark theme customization and then follow up with introducing the variables into the custom CSS in a separate PR.

@meel-hd
Copy link
Contributor Author

meel-hd commented Oct 15, 2024

@nijel the previous commit ac1289b above, makes this PR concerned only with the dark theme customization. I'll continue in another PR that depends on this one where the custom CSS variables will be consumed and used.

Copy link

codecov bot commented Oct 15, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.16%. Comparing base (a3fa4b4) to head (cb4455d).
Report is 4077 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main   #12613      +/-   ##
==========================================
+ Coverage   91.14%   91.16%   +0.01%     
==========================================
  Files         596      596              
  Lines       60938    61216     +278     
  Branches     9646     6349    -3297     
==========================================
+ Hits        55544    55807     +263     
- Misses       3723     3757      +34     
+ Partials     1671     1652      -19     
Files with missing lines Coverage Δ
weblate/wladmin/forms.py 100.00% <100.00%> (ø)
weblate/wladmin/tests.py 100.00% <100.00%> (ø)

... and 49 files with indirect coverage changes

This was used only in custom.css file. So no longer it is necessary in here
meel-hd added a commit to meel-hd/weblate that referenced this pull request Oct 16, 2024
- The custom.css consumes the custom theme colors and use them.
- Added a custom split filter for spliting the passed in 2 hex colors string.
- Added tests.

(depends on PR WeblateOrg#12613)
meel-hd added a commit to meel-hd/weblate that referenced this pull request Oct 17, 2024
This commit fixes the  test fails previously appeared. Mainly due to incorrect logic and use of incorrect color variables in the template.

Changes:
- Refactored custom.css and  made CustomCssView  handle custom theme settings instead of the css template.
- also removed the no longer used custom_filters.py and test_custom_filters.py

Dependes on PR: WeblateOrg#12613
@meel-hd meel-hd requested a review from nijel October 18, 2024 09:07
@meel-hd
Copy link
Contributor Author

meel-hd commented Oct 18, 2024

@nijel this PR and #12794 are now finished. Also note they depend on each other.

The issues with the failing tests are resolved now. lol finally 🎉

@nijel nijel merged commit 85dbb7b into WeblateOrg:main Nov 6, 2024
35 checks passed
nijel pushed a commit that referenced this pull request Nov 6, 2024
* Add custom css dark theme customization

- The custom.css consumes the custom theme colors and use them.
- Added a custom split filter for spliting the passed in 2 hex colors string.
- Added tests.

(depends on PR #12613)

* Fix test fails of custom.css theme settings

This commit fixes the  test fails previously appeared. Mainly due to incorrect logic and use of incorrect color variables in the template.

Changes:
- Refactored custom.css and  made CustomCssView  handle custom theme settings instead of the css template.
- also removed the no longer used custom_filters.py and test_custom_filters.py

Dependes on PR: #12613

* Add tests to split_colors

Fixes #9372
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.

Separate color settings for dark theme in apepearance customization
2 participants