feat: Allow changing foreground/background colors #352

Shriukan33 · 2024-01-09T11:07:14Z

name: Pull request
about: Submit a pull request for this project
assignees: fabiocaccamo

Describe your changes
I added 4 customization options to Themes :

Body background color
Foreground color
Quiet color
Loud color

And extended French translation for the newly added fields.
I couldn't find places where loud color was used, but it felt awkward to add the option for the quiet color without the loud color.

Related issue
#291

Checklist before requesting a review

I have performed a self-review of my code.
[NA] I have added tests for the proposed changes.
I have run the tests and there are no errors.

codecov · 2024-01-09T11:32:53Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (e895a05) 97.35% compared to head (d03f7e9) 97.44%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #352      +/-   ##
==========================================
+ Coverage   97.35%   97.44%   +0.08%     
==========================================
  Files          38       40       +2     
  Lines         416      430      +14     
==========================================
+ Hits          405      419      +14     
  Misses         11       11

Flag	Coverage Δ
unittests	`97.44% <100.00%> (+0.08%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

BMourguesFieldbox · 2024-01-09T11:36:06Z

Not sure why the linting fails, it seems to be related to the French translation I added, but the error isn't very clear about what's wrong...

EDIT: I compiled the new .po to .mo with msgfmt django.po -o django.mo and added it to PR

BMourguesFieldbox · 2024-01-09T14:22:44Z

The linting keeps failing, maybe I should drop the translation commit?

It's my first time updating locale, maybe I'm doing this wrong @fabiocaccamo ?

fabiocaccamo · 2024-01-09T14:42:02Z

@Shriukan33 thanks for this PR, I will review it as soon as possible!

fabiocaccamo · 2024-01-09T14:42:32Z

@BMourguesFieldbox have you followed these steps?
https://github.com/fabiocaccamo/django-admin-interface#translate-into-another-language

fabiocaccamo · 2024-01-09T14:43:30Z

This PR would probably fix #291.

BMourguesFieldbox · 2024-01-09T14:51:30Z

@BMourguesFieldbox have you followed these steps? https://github.com/fabiocaccamo/django-admin-interface#translate-into-another-language

Sorry I missed it, I've gone through the proper commands and repushed ! :)

merwok · 2024-01-09T15:22:13Z

admin_interface/locale/fr/LC_MESSAGES/django.po

@@ -174,10 +178,22 @@ msgstr "code"
 msgid "display"
 msgstr "affichage"

+#: admin_interface/models.py
+msgid "foreground color"
+msgstr "couleur du premier plan (gras, champs requis, messages d'erreur)"


It doesn’t seem correct to have this more detail in the translation and not the source strings: the English version will miss the details, and the other translators too!

So what's the play here? Should I add the details in the verbose name in the models.py file and then edit this one?
Sorry this is my first time contributing on langage :/

You should just translate the original message without adding extra informations, eg. msgstr "couleur du premier plan"

Or add the full message in English in the source code!

I went for an intermediate solution and instead I extended the help text.
Instead of adding details in the verbose name, I felt that these details belong more to the help text.

Also added the translation for those new help texts.

EDIT :
Also ran tox to check linting and it passes !

┌──(admin-interface)─(bmourgues㉿FRL167)-[~/Documents/Github/django-admin-interface/django-admin-interface] └─$ tox -e translations .pkg: _optional_hooks> python /home/bmourgues/.pyenv/versions/3.10.9/envs/admin-interface/lib/python3.10/site-packages/pyproject_api/_backend.py True setuptools.build_meta .pkg: get_requires_for_build_sdist> python /home/bmourgues/.pyenv/versions/3.10.9/envs/admin-interface/lib/python3.10/site-packages/pyproject_api/_backend.py True setuptools.build_meta .pkg: build_sdist> python /home/bmourgues/.pyenv/versions/3.10.9/envs/admin-interface/lib/python3.10/site-packages/pyproject_api/_backend.py True setuptools.build_meta translations: install_package> python -I -m pip install --force-reinstall --no-deps /home/bmourgues/Documents/Github/django-admin-interface/django-admin-interface/.tox/.tmp/package/28/django-admin-interface-0.28.3.tar.gz translations: commands[0]> python -m django makemessages --ignore .tox --ignore venv --all --add-location file --extension html,py processing locale it processing locale es processing locale fr processing locale de processing locale tr processing locale pl processing locale fa processing locale ru processing locale pt_BR translations: commands[1]> python -m django compilemessages --ignore .tox --ignore venv processing file django.po in /home/bmourgues/Documents/Github/django-admin-interface/django-admin-interface/admin_interface/locale/pl/LC_MESSAGES processing file django.po in /home/bmourgues/Documents/Github/django-admin-interface/django-admin-interface/admin_interface/locale/es/LC_MESSAGES processing file django.po in /home/bmourgues/Documents/Github/django-admin-interface/django-admin-interface/admin_interface/locale/fa/LC_MESSAGES processing file django.po in /home/bmourgues/Documents/Github/django-admin-interface/django-admin-interface/admin_interface/locale/de/LC_MESSAGES processing file django.po in /home/bmourgues/Documents/Github/django-admin-interface/django-admin-interface/admin_interface/locale/it/LC_MESSAGES processing file django.po in /home/bmourgues/Documents/Github/django-admin-interface/django-admin-interface/admin_interface/locale/tr/LC_MESSAGES processing file django.po in /home/bmourgues/Documents/Github/django-admin-interface/django-admin-interface/admin_interface/locale/ru/LC_MESSAGES processing file django.po in /home/bmourgues/Documents/Github/django-admin-interface/django-admin-interface/admin_interface/locale/pt_BR/LC_MESSAGES processing file django.po in /home/bmourgues/Documents/Github/django-admin-interface/django-admin-interface/admin_interface/locale/fr/LC_MESSAGES translations: commands[2]> git diff admin_interface/locale/ translations: commands[3]> git diff-index --quiet HEAD admin_interface/locale/ .pkg: _exit> python /home/bmourgues/.pyenv/versions/3.10.9/envs/admin-interface/lib/python3.10/site-packages/pyproject_api/_backend.py True setuptools.build_meta translations: OK (3.39=setup[2.85]+cmd[0.33,0.20,0.00,0.00] seconds) congratulations :) (3.52 seconds)

BMourguesFieldbox · 2024-01-12T13:36:32Z

@fabiocaccamo Just a reminder if you have some time to pull the trigger on the CI :)

Shriukan33 · 2024-01-23T09:39:19Z

Hey, just a heads up for this PR :)

fabiocaccamo · 2024-01-23T09:49:06Z

@Shriukan33 sorry for the delay in doing the review, but I'm quite busy these days and I'm not able to find the time.
I will do it as soon as possible, this has highest-priority in my open-source todo list.

BMourguesFieldbox · 2024-01-23T09:51:57Z

@Shriukan33 sorry for the delay in doing the review, but I'm quite busy these days and I'm not able to find the time. I will do it as soon as possible, this has highest-priority in my open-source todo list.

No problem, I understand ! Life gets in the way sometimes, good luck in whatever keeps you busy :)

selected-pixel-jameson · 2024-04-02T11:38:21Z

@fabiocaccamo Is there something blocking this from being merged? Would be awesome if we could get this in there. This project has everything I'm looking for except this feature.

fabiocaccamo · 2024-04-02T12:44:52Z

@selected-pixel-jameson I haven't had time to review and test this PR yet.

BMourguesFieldbox · 2024-04-02T12:52:00Z

@fabiocaccamo Is there something blocking this from being merged? Would be awesome if we could get this in there. This project has everything I'm looking for except this feature.

In the meantime, you could use my fork as source in your requirements like so :

django==5.*
git+https://github.com/Shriukan33/django-admin-interface.git@Allow-changing-background-color

Note that you need to have git installed on the image if you're using docker.

selected-pixel-jameson · 2024-04-02T12:53:59Z

@BMourguesFieldbox I'll take a look, but I'm kind of having concerns even moving forward with this solution as it sounds like the maintainer is potentially running into time constraint issues, which is understandable, I just don't want to implement a solution that in the end is going to run into maintenance issues.

fabiocaccamo · 2024-04-02T12:59:59Z

@BMourguesFieldbox @Shriukan33 @selected-pixel-jameson I'll try to review this as soon as possible!

BMourguesFieldbox · 2024-04-02T13:02:10Z

@BMourguesFieldbox I'll take a look, but I'm kind of having concerns even moving forward with this solution as it sounds like the maintainer is potentially running into time constraint issues, which is understandable, I just don't want to implement a solution that in the end is going to run into maintenance issues.

Understandable, this lib doesn't do much aside from offering a way to edit the admin templates, so it doesn't really relate to app's features, I'd say it's quite low risk (unless you have lots of features in the admin)

@BMourguesFieldbox @Shriukan33 @selected-pixel-jameson I'll try to review this as soon as possible!

Thanks ! :D

feat: Allow changing foreground/background colors

bf1b0f9

Shriukan33 force-pushed the Allow-changing-background-color branch from 4d57636 to 14ec150 Compare January 9, 2024 14:07

fabiocaccamo self-requested a review January 9, 2024 14:43

fabiocaccamo assigned Shriukan33 and BMourguesFieldbox Jan 9, 2024

fabiocaccamo added the enhancement New feature or request label Jan 9, 2024

MourguesBenjamin added 2 commits January 9, 2024 15:50

feat: Extend french translation

f9341fd

docs: Update README.md

b41aa18

Shriukan33 force-pushed the Allow-changing-background-color branch from e6e8206 to b41aa18 Compare January 9, 2024 14:50

merwok reviewed Jan 9, 2024

View reviewed changes

MourguesBenjamin added 2 commits January 10, 2024 11:09

feat: Extend help text for unclear css variables

b79f460

feat: Add help text translations

d03f7e9

Shriukan33 force-pushed the Allow-changing-background-color branch from d4b576f to d03f7e9 Compare January 10, 2024 10:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Allow changing foreground/background colors #352

feat: Allow changing foreground/background colors #352

Shriukan33 commented Jan 9, 2024

codecov bot commented Jan 9, 2024 •

edited

BMourguesFieldbox commented Jan 9, 2024 •

edited

BMourguesFieldbox commented Jan 9, 2024

fabiocaccamo commented Jan 9, 2024

fabiocaccamo commented Jan 9, 2024 •

edited

fabiocaccamo commented Jan 9, 2024

BMourguesFieldbox commented Jan 9, 2024

merwok Jan 9, 2024

BMourguesFieldbox Jan 9, 2024

fabiocaccamo Jan 9, 2024

merwok Jan 9, 2024

BMourguesFieldbox Jan 10, 2024 •

edited

BMourguesFieldbox commented Jan 12, 2024

Shriukan33 commented Jan 23, 2024

fabiocaccamo commented Jan 23, 2024

BMourguesFieldbox commented Jan 23, 2024

selected-pixel-jameson commented Apr 2, 2024

fabiocaccamo commented Apr 2, 2024

BMourguesFieldbox commented Apr 2, 2024

selected-pixel-jameson commented Apr 2, 2024

fabiocaccamo commented Apr 2, 2024 •

edited

BMourguesFieldbox commented Apr 2, 2024

feat: Allow changing foreground/background colors #352

Are you sure you want to change the base?

feat: Allow changing foreground/background colors #352

Conversation

Shriukan33 commented Jan 9, 2024

codecov bot commented Jan 9, 2024 • edited

Codecov Report

BMourguesFieldbox commented Jan 9, 2024 • edited

BMourguesFieldbox commented Jan 9, 2024

fabiocaccamo commented Jan 9, 2024

fabiocaccamo commented Jan 9, 2024 • edited

fabiocaccamo commented Jan 9, 2024

BMourguesFieldbox commented Jan 9, 2024

merwok Jan 9, 2024

Choose a reason for hiding this comment

BMourguesFieldbox Jan 9, 2024

Choose a reason for hiding this comment

fabiocaccamo Jan 9, 2024

Choose a reason for hiding this comment

merwok Jan 9, 2024

Choose a reason for hiding this comment

BMourguesFieldbox Jan 10, 2024 • edited

Choose a reason for hiding this comment

BMourguesFieldbox commented Jan 12, 2024

Shriukan33 commented Jan 23, 2024

fabiocaccamo commented Jan 23, 2024

BMourguesFieldbox commented Jan 23, 2024

selected-pixel-jameson commented Apr 2, 2024

fabiocaccamo commented Apr 2, 2024

BMourguesFieldbox commented Apr 2, 2024

selected-pixel-jameson commented Apr 2, 2024

fabiocaccamo commented Apr 2, 2024 • edited

BMourguesFieldbox commented Apr 2, 2024

codecov bot commented Jan 9, 2024 •

edited

BMourguesFieldbox commented Jan 9, 2024 •

edited

fabiocaccamo commented Jan 9, 2024 •

edited

BMourguesFieldbox Jan 10, 2024 •

edited

fabiocaccamo commented Apr 2, 2024 •

edited