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

Extract Translation Keys from JavaScript Files (#1713) #1716

Conversation

miguelvaara
Copy link
Contributor

@miguelvaara miguelvaara commented Dec 10, 2024

Note: @osma , this is mainly for a design review and the Design PR can still be improved if you find the design difficult to review based on this level of descriptions.

Reasons for creating this PR

Link to relevant issue(s), if any

#1713

Description of the changes in this PR

A separate PHP class (JsTranslationExtractor.php) was created. By running it automatically during the extracting and using its built-in regex expression, translation keys can be read straigth from JavaScript files and Vue components (but naturally not from Vue templates).

The functionality requires creating getters for translation keys in the computed section, as shown below:

computed: {
  testTitle () {
    return $t('Test title in js file')
  },
  termCountsTitle () {
    return $t('Term counts by language 2')
  },
}

This approach is more structured (Managing translations in computed centralizes key handling and makes it possible to process them dynamically as variables if necessary)

In practice, if this works, we won’t need to add a vue-i18n dependency which will certainly simplify future maintenance.

Intended Workflow:

  1. Read and extract new keys from Vue components.
  2. Extraction writees to the same translation files that Vue components are already reading (the simples possible implementation).

So far, the setup has been run in "dry mode" and has not yet been tested for writing to the messages files:

php bin/console translation:extract en --prefix="" --dump-messages

The keys from JavaScript test file:

php bin/console translation:extract en --prefix="" --dump-messages | grep 'Term counts by language'
Found key: Term counts by language 2
 * Term counts by language 2
 * Term counts by language

Known problems or uncertainties in this PR

The directory containing the test JavaScript file is still in the wrong location, so additional work is required to fix the paths.

Checklist

  • phpUnit tests pass locally with my changes
  • I have added tests that show that the new code works, or tests are not relevant for this PR (e.g. only HTML/CSS changes)
  • The PR doesn't reduce accessibility of the front-end code (e.g. tab focus, scaling to different resolutions, use of .sr-only class, color contrast)
  • The PR doesn't introduce unintended code changes (e.g. empty lines or useless reindentation)

@miguelvaara miguelvaara added this to the 3.0 milestone Dec 10, 2024
@miguelvaara miguelvaara requested a review from osma December 10, 2024 20:01
@miguelvaara miguelvaara self-assigned this Dec 10, 2024
…m class receiving it as an argument. And the path value is configurable in the services.yaml file. Next the code needs to be cleaned up for review
Copy link
Contributor Author

@miguelvaara miguelvaara left a comment

Choose a reason for hiding this comment

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

@osma

Remember, this for design review! :-)

The code has now been brought closer to the quality required for design review before the final code changes.

Please pay attention to the following:

Remember to run:

  • npm install
  • php composer.phar update
  • php bin/console cache:clear

Comments:

  • The location and naming of the class that enables the extraction of keys from JavaScript files (src/Translation/Extractor/JsTranslationExtractor.php).
  • To facilitate testing, additional placeholder texts "(test case [A-E])" have been added to some translation keys. Modify the letter identifiers as needed and run: php bin/console translation:extract en --prefix="" --dump-messages -vvv
  • So far the messages files have not been updated. Only "dry runs" have been performed, but the "real runs" are likely the next step to ensure proper functionality.
  • The JavaScript files directory is provided as an argument in the services.yaml file, so hardcoding at the code level was avoided.
  • A logical next step for the following issue would be configuring the push to Lokalise. Does the work done here support that goal?

///// UPDATE

I just realized that even the code works, I have handled the directory variable in the class (src/Translation/Extractor/JsTranslationExtractor.php) somewhat inefficiently. The code will definitely need some adjustments and redundancy in the checks should be minimized. PHP's less functional approach and the use of global variables within a class require some getting used to, specifically deciding whether to pass something as a parameter to a method or to refer directly to a global variable within the method. I will streamline the code!

Copy link

codecov bot commented Dec 12, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 73.13%. Comparing base (84b37b4) to head (5c6ff5a).
Report is 25 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff              @@
##               main    #1716      +/-   ##
============================================
+ Coverage     70.91%   73.13%   +2.22%     
- Complexity     1651     1958     +307     
============================================
  Files            33       33              
  Lines          4332     4769     +437     
============================================
+ Hits           3072     3488     +416     
- Misses         1260     1281      +21     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@miguelvaara miguelvaara marked this pull request as ready for review December 12, 2024 11:45
Copy link
Contributor Author

@miguelvaara miguelvaara left a comment

Choose a reason for hiding this comment

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

Review Instructions

Copy and run the following:

git fetch
git switch issue1713-what-is-the-best-way-to-extract-translation-keys-from-vue-components
git pull
php composer.phar update
npm install

Translation key extraction from Vue components

You can test the functionality by modifying a translation key in the file resource/js/term-counts.js. For example, change the key return $t('Term counts by language'). Then run the command php bin/console translation:extract en --prefix="" --force --format=json in the root of the Skosmos project. This will add the modified translation key to the file resource/translations/messages.en.json.

Additional Note:

The tests SonarCloud Code Analysis and codecov/project are failing. How do you approach this?

Copy link
Member

@osma osma left a comment

Choose a reason for hiding this comment

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

It works as it should, but I think the code needs some further cleanup.
Also, I suggested renaming the file to better fit in with Skosmos file naming conventions.

src/Translation/Extractor/JsTranslationExtractor.php Outdated Show resolved Hide resolved
src/Translation/Extractor/JsTranslationExtractor.php Outdated Show resolved Hide resolved
src/Translation/Extractor/JsTranslationExtractor.php Outdated Show resolved Hide resolved
src/Translation/Extractor/JsTranslationExtractor.php Outdated Show resolved Hide resolved
src/Translation/Extractor/JsTranslationExtractor.php Outdated Show resolved Hide resolved
Copy link
Member

@osma osma left a comment

Choose a reason for hiding this comment

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

LGTM

@miguelvaara miguelvaara merged commit 30d6246 into main Dec 12, 2024
10 of 11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Issue/PR closed
Development

Successfully merging this pull request may close these issues.

Sub of #1712: What is the best way to extract translation keys from Vue components?
2 participants