-
-
Notifications
You must be signed in to change notification settings - Fork 174
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: Allow changing foreground/background colors #352
base: main
Are you sure you want to change the base?
feat: Allow changing foreground/background colors #352
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
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
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
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 |
4d57636
to
14ec150
Compare
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 ? |
@Shriukan33 thanks for this PR, I will review it as soon as possible! |
@BMourguesFieldbox have you followed these steps? |
This PR would probably fix #291. |
e6e8206
to
b41aa18
Compare
Sorry I missed it, I've gone through the proper commands and repushed ! :) |
@@ -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)" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 :/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should just translate the original message without adding extra informations, eg. msgstr "couleur du premier plan"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or add the full message in English in the source code!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
d4b576f
to
d03f7e9
Compare
@fabiocaccamo Just a reminder if you have some time to pull the trigger on the CI :) |
Hey, just a heads up for this PR :) |
@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. |
No problem, I understand ! Life gets in the way sometimes, good luck in whatever keeps you busy :) |
@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. |
@selected-pixel-jameson I haven't had time to review and test this PR yet. |
In the meantime, you could use my fork as source in your requirements like so :
Note that you need to have git installed on the image if you're using docker. |
@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. |
@BMourguesFieldbox @Shriukan33 @selected-pixel-jameson I'll try to review this as soon as possible! |
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)
Thanks ! :D |
name: Pull request
about: Submit a pull request for this project
assignees: fabiocaccamo
Describe your changes
I added 4 customization options to Themes :
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