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

TLN Add missing js translation strings to src/en.json file #1702

Merged
merged 1 commit into from Mar 7, 2024

Conversation

pschiffe
Copy link
Contributor

@pschiffe pschiffe commented Mar 6, 2024

Description

I went through the source code in the client/src dir and addded all missing translation strings to the client/lang/src/en.json file.

Manual testing steps

Issues

Pull request checklist

  • The target branch is correct
  • All commits are relevant to the purpose of the PR (e.g. no debug statements, unrelated refactoring, or arbitrary linting)
    • Small amounts of additional linting are usually okay, but if it makes it hard to concentrate on the relevant changes, ask for the unrelated changes to be reverted, and submitted as a separate PR.
  • The commit messages follow our commit message guidelines
  • The PR follows our contribution guidelines
  • Code changes follow our coding conventions
  • This change is covered with tests (or tests aren't necessary for this change)
  • Any relevant User Help/Developer documentation is updated; for impactful changes, information is added to the changelog for the intended release
  • CI is green

@pschiffe
Copy link
Contributor Author

pschiffe commented Mar 6, 2024

I have one more question. I also found the following string IDs - where do they belong?

Boolean.ANY
CampaignAdmin.DELETECAMPAIGN
CampaignAdmin.LOADING
CampaignAdmin.NO_RECORDS
PopoverOptionSet.CLEAR
PopoverOptionSet.NO_RESULTS
PopoverOptionSet.SEARCH_PLACEHOLDER
PopoverOptionSetToggle.TOGGLE

and

SilverStripe\\Admin\\FileStatusIcon.TRACKED_FORM_UPLOAD_RESTRICTED
SilverStripe\\Admin\\FileStatusIcon.TRACKED_FORM_UPLOAD_UNRESTRICTED
SilverStripe\\Admin\\FileStatusIcon.ACCESS_RESTRICTED

Copy link
Member

@GuySartorelli GuySartorelli left a comment

Choose a reason for hiding this comment

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

Fantastic effort, thank you. I'll merge this once CI goes green.

Just as a housekeeping note, please don't tick checkboxes that aren't appropriate to tick - for example you have ticked "CI is green" but there are two failing builds.
It makes it hard to trust that any of the other checkboxes are correctly ticked 😅

Please also link to an open issue, not a closed one. We can't track closed issues.

In this case the CI failures are probably intermittent so I've rerun them.

I have one more question. I also found the following string IDs - where do they belong?

That depends where you found them, and whether they're declared in any other modules or not.

@pschiffe
Copy link
Contributor Author

pschiffe commented Mar 7, 2024

Thanks for taking a look and sorry for my lousiness 🙈

All the string IDs were found in the client/src folder. Is it OK to have cross module translation dependencies like.. will it work?

Also, if the string ID is SilverStripe\\Admin\\FileStatusIcon.ACCESS_RESTRICTED in the javascript file, will it be read from the translation yaml, or should it be also added to the client/lang/src/en.json file?

@pschiffe
Copy link
Contributor Author

pschiffe commented Mar 7, 2024

Hm, this file probably also requires to rebuild the bundle. Please help me to sort out the missing string IDs and I'll update the PR 🙏

 git diff found modified files when it should not have:
M js/bundle.js
sha1sum of files that are different:
a790abc90e872e05680c06101c0adfc34df9c5fa  client/dist/js/bundle.js

@GuySartorelli
Copy link
Member

All the string IDs were found in the client/src folder.

If they're not already declared by other modules, then they will need to be added to the json file.

The CampaignAdmin ones almost certainly are specific to the campaign admin module for example... it's suspicious that they're here.

Maybe open a new separate issue about the ones you're not sure about, and link to where each one is in the code, so we can discuss where they belong without delaying merging this PR which is good to go as-is.

Is it OK to have cross module translation dependencies like.. will it work?

If the module is installed, it will pull the translations from that module.
If the module is not installed, it will fall back to the default string which is declared in the js.

In short: yes ;p

Also, if the string ID is SilverStripe\\Admin\\FileStatusIcon.ACCESS_RESTRICTED in the javascript file, will it be read from the translation yaml, or should it be also added to the client/lang/src/en.json file?

The yaml file for PHP is entirely separate. Unfortunately.

@GuySartorelli
Copy link
Member

The JS build failure here is unrelated to the changes in this PR.

I went through the source code in the `client/src` dir and addded all
missing translation strings to the `client/lang/src/en.json` file.

Related silverstripe#1689
@pschiffe
Copy link
Contributor Author

pschiffe commented Mar 7, 2024

@GuySartorelli I've updated the PR and added all missing string IDs except the CampaignAdmin ones, which should be in the campaign admin module I think. I didn't find the others, the Boolean and PopoverOptionSet anywhere else, so I think they are OK here.

I'm happy with this PR now.

Copy link
Member

@GuySartorelli GuySartorelli left a comment

Choose a reason for hiding this comment

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

LGTM. Thank you for making these changes.
They'll be reflected in transifex next time we do a translations run.

@GuySartorelli GuySartorelli merged commit 70c4589 into silverstripe:1.13 Mar 7, 2024
12 checks passed
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.

None yet

2 participants