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

PB-227: Update language #86

Merged
merged 3 commits into from
Sep 11, 2024
Merged

PB-227: Update language #86

merged 3 commits into from
Sep 11, 2024

Conversation

LukasJoss
Copy link
Contributor

No description provided.

@LukasJoss
Copy link
Contributor Author

@hansmannj @pakb What do you think of renaming the babs foldes to that they use iso-code language abbreviations (e.g babs-I -> babs-it). I prefer it, but it is no longer consistent with the names of the icons

app/icon_set.py Outdated
@@ -121,5 +121,6 @@ def serialize(self):
"colorable": self.colorable,
"icons_url": self.get_icons_url(),
"template_url": get_icon_set_template_url(get_base_url()),
"has_description": bool(find_descripton_file(self.name))
"has_description": bool(find_descripton_file(self.name)),
"language": self.name.split('-')[-1] if '-' in self.name else False
Copy link
Member

Choose a reason for hiding this comment

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

Not sure, if this logic will do for all cases.
For example a file called "001-Beschaedigung.png" would lead to a wrong language.

Copy link
Member

Choose a reason for hiding this comment

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

We might take the language from the directory or the icon-set name respectively.
In that case we need to make sure, that the language is always part of the icon set name.
Why do we need the language in the first place?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is for the icon set, not the icons

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we need the language so we can properly translate the icon set selector dropdown on the mapviewer

Copy link
Member

Choose a reason for hiding this comment

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

Ah, stupid me 🙈
Of course, that makes sense then!

Copy link
Member

Choose a reason for hiding this comment

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

In this case, we need to document and make sure, that all (new) icon sets are always named correctly and don't include - unless for adding a language.

Copy link
Member

Choose a reason for hiding this comment

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

That touches our architectural decissions @boecklic

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can also use a dictionary if that is preferable

Copy link
Contributor Author

@LukasJoss LukasJoss Sep 11, 2024

Choose a reason for hiding this comment

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

I have replaced it with a dictionary for now, we can change it back later if necessary

Copy link
Member

Choose a reason for hiding this comment

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

Thanks! I had a similar idea later yesterday evening.
Slightly diferent and a bit more generic, without having to hardcode the icon-set's languages.

We could have a list with the languages we support, i.e. SUPPORTED_LANGUAGES = ["FR", "IT", "RM", "DE", "EN"] for example.
Then we can use your approach you had in a few commits before, i.e. trying to get the icon-set's language from the name/path. Then check, if the derived language is in our list of supported languages.
If it is: bingo, else None for the language.
How about that?

@LukasJoss LukasJoss merged commit 8cc7db3 into develop Sep 11, 2024
3 checks passed
@LukasJoss LukasJoss deleted the feat-PB-227-update-language branch September 11, 2024 11:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants