-
Notifications
You must be signed in to change notification settings - Fork 32
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 color to language model #2715
base: develop
Are you sure you want to change the base?
Conversation
I am not sure whether I am doing it right or not. I would be glad to get some guidance :) |
Code Climate has analyzed commit 9924d25 and detected 0 issues on this pull request. The test coverage on the diff in this pull request is 100.0% (50% is the threshold). This pull request will bring the total coverage in the repository to 81.9% (-0.2% change). View more on Code Climate. |
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.
Thank you so much for the PR 💪
You are already in a good direction 😸 There are some suggestions (find in comments) 👍
@lunars97 |
…del' into feature/add-color-to-language-model
0c2ad59
to
c9d70a5
Compare
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.
Looks good 😸 Now we can adapt the matomo_api_client.py 💪 Find the comments please.
A new specification came after the Service-UI/UX-CMS monthly meeting on the last monday: only colors should be shown as choice which are not yet assigned to any language so one doesn't have to check the list every time when assigning a color to another language 🙏 |
Can I then remove blocked and ui/ux labels and continue working on it? |
@osmers If the language has no color, the blank choice will be shown and when clicked on the drop down, the choice appears including colors that are assigned to other languages but with note which language is using the color If the language has a color, it looks like this first After clicking on the drop dow In this way it's easier to swap colors (because one doesn't have to think about which color is free for a temporary assigment during the swap) and also you can see directly in the choice which color and language are paired.
@lunars97 I |
Looks good and I like the fact, that we can see which color belongs to which language. I would also suggest to make the color field mandatory so that we do not end up with a language without color, ok? |
@MizukiTemma thank you for improving it :) |
I am receiving test error. Could someone help me to identify the testing file which is causing current error. I would appreciate it a lot :) |
Yup, we can do that 👍 |
@@ -54,6 +56,29 @@ def __init__(self, *args: Any, **kwargs: Any) -> None: | |||
self.fields["primary_country_code"].choices = sorted_language_choices | |||
self.fields["secondary_country_code"].choices = sorted_language_choices | |||
|
|||
# Make the language color field required | |||
self.fields["language_color"].required = True |
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.
@MizukiTemma I added required attribute here. Is that okay? and seems like it is causing testing error.
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.
Thank you for updates 👍 Yes, we have to adjust the tests. language_color
is a new field and therefore not correctly handled in the test, that's the cause (I guess).
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.
@MizukiTemma yep, that is what is saying, but I cannot identify the file where I should add that field :(
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 suggest to remove blank=True
and null=True
from the language_color
in the model to allow no language to exist without color, before adjusting the tests.
But then we have to assign a color to each existing language in the migration file (otherwise it goes broken since languages stored in the database don't have any color so far), and the design for the color-language pairs can be now changed, because we asked for color/design for total access numbers too.
So, let's wait 😉
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.
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.
@MizukiTemma ahh I see now. I tried filling the necessary objects in test_data.json without adding it as well to view_config.py and that way I was also getting errors. Now I see why, thank you a lot :)
Sounds reasonable 👍 I've totally forgotten the issue 😅 |
I have added colors for WebApp Access and Offline Access to the original 🎨 figma file. Thanks for being so patient! |
@MizukiTemma hmm do we also need for Total Access? Currently we do not have a designated color for it, it uses default color, can I designate the default to total access then? EDIT: I assigned the default color for TotalAccess. If it is okay, then we can leave it as it is... |
…color labels round
You can also use black (#000000) for Total Access. I have added it to the design. |
6a08f3d
to
8a86bc1
Compare
63ebe99
to
84b3169
Compare
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.
Thank you a lot for this PR. You did a very good job. I have a few suggestion and after that I think this PR is ready for release 🚀
@@ -9,7 +9,7 @@ class Migration(migrations.Migration): | |||
""" | |||
|
|||
dependencies = [ | |||
("cms", "0088_rename_mt_related_fields"), | |||
("cms", "0089_add_language_color"), |
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.
("cms", "0089_add_language_color"), | |
("cms", "0088_rename_mt_related_fields"), |
""" | ||
|
||
dependencies = [ | ||
("cms", "0088_rename_mt_related_fields"), |
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.
("cms", "0088_rename_mt_related_fields"), | |
("cms", "0089_add_language_color"), |
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.
Please rename this file to 0090_... (or even a later number if 90 is already taken by now) :)
|
||
#: cms/constants/language_color.py | ||
msgid "Green blue" | ||
msgstr "Grün-blau" |
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.
msgstr "Grün-blau" | |
msgstr "Grünblau" |
Duden says no dash in between :)
|
||
#: cms/constants/language_color.py | ||
msgid "Pink orange" | ||
msgstr "Rosa-orange" |
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.
Not found in Duden, probably rosaorange? But it's a newly created word as far as Duden is concerned 😅
@@ -0,0 +1,2 @@ | |||
en: Add color to the language model |
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.
en: Add color to the language model | |
en: Add fixed colors to the languages in the statistics |
@@ -0,0 +1,2 @@ | |||
en: Add color to the language model | |||
de: Füge einer Farbe zum Sprachmodell hinzu |
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.
de: Füge einer Farbe zum Sprachmodell hinzu | |
de: Füge den Sprachen in den Statistiken eine feste Farbe hinzu |
|
||
#: cms/models/languages/language.py | ||
msgid "This is used to represent the color label of the chosen language" | ||
msgstr "Diese Flagge wird verwendet, um die Sprache graphisch darzustellen." |
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.
msgstr "Diese Flagge wird verwendet, um die Sprache graphisch darzustellen." | |
msgstr "Diese Farbe wird verwendet, um die Sprache graphisch darzustellen." |
used_colors = [] | ||
unused_colors = [] | ||
for color in language_color.COLORS: | ||
code, name = color |
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.
code, name = color | |
color_code, name = color |
I'd rather name this color_code to be more specific, but I'll leave it up to you to decide :)
@@ -0,0 +1,20 @@ | |||
const renderLanguageColorPreview = ({ target }: Event) => { | |||
const colorSelect = target as HTMLSelectElement; |
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.
To be honest, I don't fully understand the difference between selectedColor and colorSelect. Could you please find a better naming for either one? :)
19bd128
to
9409657
Compare
9409657
to
9924d25
Compare
Short description
Add static color labels to language model in order to improve understanding of statistics regardless of to which region it belongs.
Proposed changes
Side effects
None?
Additional info
Should be done before #1097
Resolved issues
Fixes: #2390
Pull Request Review Guidelines