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

[UI] Localizing level keywords #4846

Merged
merged 7 commits into from
Dec 7, 2023
Merged

[UI] Localizing level keywords #4846

merged 7 commits into from
Dec 7, 2023

Conversation

Annelein
Copy link
Collaborator

@Annelein Annelein commented Dec 4, 2023

Fixes #4570

How to test: Go to the My Programs page, click 'Adventures', the command adventures are now localized.

@ghost
Copy link

ghost commented Dec 4, 2023

👇 Click on the image for a new way to code review

Review these changes using an interactive CodeSee Map

Legend

CodeSee Map legend

@Annelein Annelein marked this pull request as ready for review December 4, 2023 19:26
@Felienne
Copy link
Member

Felienne commented Dec 5, 2023

Hi @Annelein,

I don't see the problem being fixed yet:

image

But maybe I am not testing in the right way? Adding a "How to test" section to your PRs (as is also present in the template) helps people to know what to see, and understand the change a bit better.

@Felienne
Copy link
Member

Felienne commented Dec 5, 2023

Ah I see! You have translated the names but left the curlies around them:

image image

I did not see that from print since print in Dutch is also print haha.

You see now how useful the How to test is :)

I think I would prefer we remove the curlies too? They don't look pretty and they are used to indicate places where localization can happen internally.

hedy_content.py Outdated
for key, value in adventure_names.items():
if 'command' in key:
words = value.split()
adventure_names[key] = ' '.join(['{' + word + '}' if not word.isdigit()
Copy link
Member

Choose a reason for hiding this comment

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

Because we already have the curlies in the string, you can actually do this in an easier way, by applying a dictionary to the string directly! I don't know the syntax by heart but it occurs in a number of places in the code. Let mw know if you need help!

Copy link
Member

Choose a reason for hiding this comment

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

I think it is this function:

def safe_format(s: str, /, **kwargs):

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't think there are curlies in the string yet right? Because it retrieves 'Slaap' from here? Or maybe I don't really understand what you mean?

Screenshot 2023-12-05 at 09 29 48

@Annelein
Copy link
Collaborator Author

Annelein commented Dec 5, 2023

@Felienne sorry I didn't add a test! I wanted to add a screenshot but I can't screenshot the Adventure options on the MyProgram page... So forgot to write a how to test haha.

@Annelein
Copy link
Collaborator Author

Annelein commented Dec 5, 2023

@Felienne I added the curlies because I thought it would be nice to indicate keyword adventures. Because like its done here:

Screenshot 2023-12-05 at 09 19 39

But I understand now the curlies are for where localization can happen internally. Let me know if you still think I should add something as an indication.

@Felienne
Copy link
Member

Felienne commented Dec 5, 2023

@Felienne I added the curlies because I thought it would be nice to indicate keyword adventures. Because like its done here:

Screenshot 2023-12-05 at 09 19 39 But I understand now the curlies are for where localization can happen internally. Let me know if you still think I should add something as an indication.

That is a great question, should we indicate it in any way? I would think it is fine to not do it, if you want to (and if it is possible) we could add a subtle link line in the dropdown? But I can totally live without it!

@Annelein
Copy link
Collaborator Author

Annelein commented Dec 5, 2023

@Felienne I removed the curlies, so I think it is ready!

Copy link
Member

@Felienne Felienne left a comment

Choose a reason for hiding this comment

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

Works perfectly now!

image

Copy link
Contributor

mergify bot commented Dec 7, 2023

Thank you for contributing! Your pull request is now going on the merge train (choo choo! Do not click update from main anymore, and be sure to allow changes to be pushed to your fork).

@mergify mergify bot merged commit ebd152a into main Dec 7, 2023
11 checks passed
@mergify mergify bot deleted the localized-keywords branch December 7, 2023 03:42
Copy link
Contributor

mergify bot commented Dec 7, 2023

Thank you for contributing! Your pull request is now going on the merge train (choo choo! Do not click update from main anymore, and be sure to allow changes to be pushed to your fork).

@Annelein Annelein self-assigned this Dec 8, 2023
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.

[CONTENT] on "My programs" page, localization does not work for Adventure names
2 participants