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 color to language model #2715

Merged
merged 1 commit into from
Jun 10, 2024
Merged

Conversation

lunars97
Copy link
Contributor

@lunars97 lunars97 commented Mar 28, 2024

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

  • Add static representative colors to languages
  • Add color field to language model like in Poi Category
  • Make the color labels in statistic and in other components round

Side effects

None?

Additional info

Should be done before #1097

Resolved issues

Fixes: #2390


Pull Request Review Guidelines

@lunars97 lunars97 requested a review from a team as a code owner March 28, 2024 16:28
@lunars97
Copy link
Contributor Author

I am not sure whether I am doing it right or not. I would be glad to get some guidance :)

Copy link

codeclimate bot commented Mar 28, 2024

Code Climate has analyzed commit 84d0e6e 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 82.6% (0.0% change).

View more on Code Climate.

Copy link
Member

@MizukiTemma MizukiTemma left a 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) 👍

integreat_cms/cms/models/languages/language.py Outdated Show resolved Hide resolved
integreat_cms/cms/constants/language_color.py Outdated Show resolved Hide resolved
@lunars97 lunars97 requested a review from MizukiTemma April 2, 2024 16:06
@MizukiTemma
Copy link
Member

@lunars97
Thank you for the updates 🙂
Could you add a color column in the region language tree (in the first picture, like the language list of network management) and a color circle or something like that in the region language node form (second picture, like the country flag)?

region language tree
language node form

@lunars97 lunars97 force-pushed the feature/add-color-to-language-model branch from 0c2ad59 to c9d70a5 Compare April 3, 2024 11:15
@lunars97 lunars97 requested a review from MizukiTemma April 3, 2024 11:32
Copy link
Member

@MizukiTemma MizukiTemma left a 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.

integreat_cms/cms/constants/language_color.py Show resolved Hide resolved
integreat_cms/matomo_api/matomo_api_client.py Outdated Show resolved Hide resolved
@JoeyStk JoeyStk added question Further information is requested ui-ux Issues that requires an UI/UX perspective. blocked Blocked by external dependency and removed question Further information is requested labels Apr 8, 2024
@MizukiTemma
Copy link
Member

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 🙏

@lunars97
Copy link
Contributor Author

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?

@lunars97 lunars97 removed ui-ux Issues that requires an UI/UX perspective. blocked Blocked by external dependency labels Apr 15, 2024
@MizukiTemma
Copy link
Member

MizukiTemma commented Apr 16, 2024

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 🙏

@osmers
To this thema there is a suggestion as below and we want to hear your opinion.

If the language has no color, the blank choice will be shown
Screenshot (2)

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
Screenshot (3)

If the language has a color, it looks like this first
Screenshot (4)

After clicking on the drop dow
Screenshot (5)

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.

  • The demonstration above was done with less color choices to make screen shot of the choice list easier

@lunars97 I will push pushed this suggestion

@osmers
Copy link

osmers commented Apr 17, 2024

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 🙏

@osmers To this thema there is a suggestion as below and we want to hear your opinion.

If the language has no color, the blank choice will be shown Screenshot (2)

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 Screenshot (3)

If the language has a color, it looks like this first Screenshot (4)

After clicking on the drop dow Screenshot (5)

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.

  • The demonstration above was done with less color choices to make screen shot of the choice list easier

@lunars97 I will push pushed this suggestion

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?

@lunars97
Copy link
Contributor Author

@MizukiTemma thank you for improving it :)
Should we leave statistics page for now as it is? Because the proposed final design of the page on ticket #1097 is like this
image

@lunars97
Copy link
Contributor Author

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 :)

@MizukiTemma
Copy link
Member

@osmers

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?

Yup, we can do that 👍

@MizukiTemma
Copy link
Member

@MizukiTemma thank you for improving it :) Should we leave statistics page for now as it is? Because the proposed final design of the page on ticket #1097 is like this image

Sounds reasonable 👍 I've totally forgotten the issue 😅

@lunars97 lunars97 added ui-ux Issues that requires an UI/UX perspective. blocked Blocked by external dependency labels Apr 18, 2024
@hauf-toni
Copy link

I have added colors for WebApp Access and Offline Access to the original 🎨 figma file. Thanks for being so patient!

@lunars97 lunars97 removed ui-ux Issues that requires an UI/UX perspective. blocked Blocked by external dependency labels May 8, 2024
@lunars97
Copy link
Contributor Author

lunars97 commented May 8, 2024

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...

@lunars97 lunars97 added the question Further information is requested label May 8, 2024
@hauf-toni
Copy link

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...

You can also use black (#000000) for Total Access. I have added it to the design.

@lunars97 lunars97 removed the question Further information is requested label May 13, 2024
@lunars97 lunars97 force-pushed the feature/add-color-to-language-model branch from 6a08f3d to 8a86bc1 Compare May 22, 2024 11:23
@lunars97 lunars97 requested a review from MizukiTemma May 22, 2024 11:34
@lunars97 lunars97 force-pushed the feature/add-color-to-language-model branch from 63ebe99 to 84b3169 Compare May 27, 2024 07:48
Copy link
Contributor

@JoeyStk JoeyStk left a 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 🚀

integreat_cms/cms/migrations/0090_add_poicategory_icons.py Outdated Show resolved Hide resolved
integreat_cms/cms/migrations/0089_add_language_color.py Outdated Show resolved Hide resolved
integreat_cms/cms/migrations/0089_add_language_color.py Outdated Show resolved Hide resolved
integreat_cms/locale/de/LC_MESSAGES/django.po Outdated Show resolved Hide resolved
integreat_cms/locale/de/LC_MESSAGES/django.po Outdated Show resolved Hide resolved
integreat_cms/release_notes/current/unreleased/2390.yml Outdated Show resolved Hide resolved
integreat_cms/release_notes/current/unreleased/2390.yml Outdated Show resolved Hide resolved
integreat_cms/locale/de/LC_MESSAGES/django.po Outdated Show resolved Hide resolved
integreat_cms/cms/forms/languages/language_form.py Outdated Show resolved Hide resolved
@lunars97 lunars97 force-pushed the feature/add-color-to-language-model branch 2 times, most recently from 9409657 to 9924d25 Compare May 27, 2024 14:41
@lunars97 lunars97 requested a review from JoeyStk May 27, 2024 14:51
@JoeyStk JoeyStk self-assigned this May 28, 2024
Copy link
Contributor

@JoeyStk JoeyStk left a 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! Looks good to me now :)

I didn't double-check all of the color names, only a few that didn't seem right to me. I hope that's okay and we only need to adjust it if we get some feedback :)

@MizukiTemma
Copy link
Member

MizukiTemma commented Jun 7, 2024

@lunars97 Thank you so much 😸 it looks good to go!

Could you squash the commits and rebase?

@MizukiTemma MizukiTemma self-assigned this Jun 7, 2024
@lunars97 lunars97 force-pushed the feature/add-color-to-language-model branch 2 times, most recently from ce5c847 to 9924d25 Compare June 10, 2024 08:07
@JoeyStk JoeyStk force-pushed the feature/add-color-to-language-model branch from 9924d25 to 84d0e6e Compare June 10, 2024 09:05
Copy link
Member

@MizukiTemma MizukiTemma left a comment

Choose a reason for hiding this comment

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

🎉

@lunars97 lunars97 merged commit df861ec into develop Jun 10, 2024
5 checks passed
@lunars97 lunars97 deleted the feature/add-color-to-language-model branch June 10, 2024 09:49
@JoeyStk JoeyStk mentioned this pull request Jun 18, 2024
3 tasks
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.

Add color to Language model
5 participants