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

Let CSS do the uppercase transformation. #1521

Merged
merged 1 commit into from
Oct 19, 2022
Merged

Conversation

kittaakos
Copy link
Contributor

@kittaakos kittaakos commented Oct 4, 2022

Motivation

Do not expose implementation details in the translation files. Related comment: #1341 (comment)

This PR is a follow-up of #1507. Use no uppercase letters. CSS will do the trick here and here.

This PR also changes the default translations of the following UI labels as requested in #1521 (review):

  • GO TO CLOUD ->. Go to Cloud, and
  • INSTALL -> Install,

Change description

To verify:

  • Open the library/boards manager widget, and if you do not have any libs/cores installed install one or two.
    • During the installation, make sure the Install button shows INSTALL although the translation file contains Install,
    • After installing a few libs/cores, you should see INSTALLED and not Installed for the items,
    • When you hover over INSTALLED, you should see UNINSTALL (assuming your IDE2 language is English)
  • The cloud verification is a bit trickier as the GO TO CLOUD is only visible if you are logged in to Arduino Create but do not have any cloud sketches. Ensuring zero remote sketches was difficult as the Create editor forced me to have at least one sketch, but I found a glitch and could workaround this behavior. When there is a single sketch, delete it, reload the page, and delete it again, then Create Editor fails to create a new resource. Here is a screencast with the steps:
deleta_all_create_sketch.mp4

Here are the changes in action in IDE2:

INSTALL/INSTALLED/UNINSTALL:

install_installed_uninstall_uppercase.mp4

GO TO CLOUD:

go_to_cloud.mp4

Other information

No behavioral changes are expected.

Reviewer checklist

  • PR addresses a single concern.
  • The PR has no duplicates (please search among the Pull Requests before creating one)
  • PR title and description are properly filled.
  • Docs have been added / updated (for bug fixes / features)

@kittaakos kittaakos added type: enhancement Proposed improvement topic: code Related to content of the project itself labels Oct 4, 2022
Copy link
Contributor

@AlbyIanna AlbyIanna left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@per1234 per1234 left a comment

Choose a reason for hiding this comment

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

It appears the remaining two uppercase strings can also be switched to normal case and the existing code will handle the styling:

{nls.localize('arduino/component/install', 'INSTALL')}

@kittaakos
Copy link
Contributor Author

OK. Let's do the cleanup everywhere. 👍

Expose no implementation details to translation files.

Signed-off-by: Akos Kitta <[email protected]>
onClick={() => shell.openExternal('https://create.arduino.cc/editor')}
>
{nls.localize('cloud/GoToCloud', 'GO TO CLOUD')}
{nls.localize('arduino/cloud/goToCloud', 'Go to Cloud')}
Copy link
Contributor

Choose a reason for hiding this comment

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

Changing the localization key from 'cloud/GoToCloud' to 'arduino/cloud/goToCloud' is right, but what about all the translations of this string? Like this we would lose them all.

Copy link
Contributor

Choose a reason for hiding this comment

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

I have also thought about the impact of prior changes to the keys. It was previously discussed here:

#1431 (comment)

I think there is an impact, but it can't quite be described as "lose them" because the translations are still stored in the "translation memory" on Transifex. So they don't need to be translated over again from English, but I don't think this happens automatically, meaning there is still an impact. I believe the translators will still need to select the strings with changed keys and then accept the auto-completed translation offered by Transifex.

Copy link
Contributor

Choose a reason for hiding this comment

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

So it seems to me that we should avoid frivolous key changes, but also shouldn't feel locked into the existing key names in cases where there is a significant benefit to restructuring them.

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 think this is the perfect time to make such changes. I do not want to maintain a key that does make sense in the next two decades.

Copy link
Contributor

@per1234 per1234 left a comment

Choose a reason for hiding this comment

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

Thanks Akos! I think comprehensively establishing this convention now will ensure the prior inconsistency doesn't end up being propagated into strings added over time and will improve the contributor experience for translators.

@kittaakos kittaakos merged commit 87ebcbe into main Oct 19, 2022
@kittaakos kittaakos deleted the #1507-no-uppercase branch October 19, 2022 08:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: code Related to content of the project itself type: enhancement Proposed improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants