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

Translation functionality in Twig templates #1690

Merged

Conversation

miguelvaara
Copy link
Contributor

@miguelvaara miguelvaara commented Oct 3, 2024

Implements translation functionaities centrally in JavaScript on the front end.

The implementation avoids using the vue-i18n library, aiming to reduce dependencies by relying on vanilla JavaScript. This is the first workin version, which can be improved in future iterations. Cypress tests were also added for the tables on the landing page.

Reasons for creating this PR

Translations also need to work on the frontend side and Vue components

Link to relevant issue(s), if any

Description of the changes in this PR

  • Created a translation service component
  • The translation service is invoked and used in two components (vocab-counts.js / term-counts.js) so far
  • Code has been modified towards the use of anonymous functions

Known problems or uncertainties in this PR

Checklist

  • [ x] phpUnit tests pass locally with my changes
  • [ x] 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)
  • [ x] 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)
  • [ x] The PR doesn't introduce unintended code changes (e.g. empty lines or useless reindentation)

…re translations are visible in the browser and the translation is read at the correct point in the code
…onent is used, which is then imported into the components where translations are needed. In such an implementation, Vue components must be called as modules
@miguelvaara miguelvaara self-assigned this Oct 4, 2024
@miguelvaara miguelvaara added this to the 3.0 milestone Oct 4, 2024
@osma
Copy link
Member

osma commented Oct 9, 2024

Doing the initialization of Vue components like this is a somewhat invasive change, but I'll defer the discussion about this.

However, I think it's problematic that each component has to call createI18nInstance separately. This means that the i18n service is initialized several times and also the messages will be loaded many times, as shown here in the Network view:

image

I think it would be better to initialize the service just once per page and make it available to all Vue components, without each component having to initialize it on its own.

@miguelvaara
Copy link
Contributor Author

miguelvaara commented Oct 9, 2024

Now there is an implementation where both statistics (vocab-counts & term-counts) on the vocabulary homepage are displayed at the same time, but the translation initialization is only done once (see image). It was a long and frustrating process where only one of the Vue apps would load at a time 😅. The issue is that I had to build a separate timing function since Vue now relies on the translation service. The timing conflict is now resolved with a separate timing function 🫣. The setup works, but generally speaking, when using frameworks, it feels a bit unsophisticated. I will not commit yet - I eill clean up the code first and make sure I fully understand what "spaghetti" have appearead through my own hands! 😂
Screenshot from 2024-10-09 15-41-02

… agreed on in the design review. Also made some minor refactorings related to the translations themselves, as they take part in the translation functionalities, plus a few other small adjustments. However, the primary changes involve significant structural modifications related to call methods, wrappings, and timing
Copy link
Contributor

@UnniKohonen UnniKohonen left a comment

Choose a reason for hiding this comment

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

This solution works and seems fine for now. In the future, we can think about using the vue-i18n library if it adds any value. It would eliminate the need to check the availability of the $t function, but there would be some blinking with the component texts. Also, it might be worth investigating if promises could be used instead of setTimeout when checking for $t and starting Vue components. Though, based on my brief testing and googling it seems that setTimeout is the most straightforward way to wait for the function to be available.

@miguelvaara miguelvaara reopened this Oct 10, 2024
@miguelvaara miguelvaara marked this pull request as ready for review October 10, 2024 11:29
@miguelvaara miguelvaara force-pushed the issue1523translate-messages-shown-by-vue-components-2 branch from 113544d to 898a64e Compare October 10, 2024 12:02
Copy link

sonarcloud bot commented Oct 10, 2024

Quality Gate Failed Quality Gate failed

Failed conditions
58.1% Duplication on New Code (required ≤ 3%)

See analysis details on SonarCloud

@miguelvaara miguelvaara changed the title The first version of translation functionality in Twig templates, whe… Translation functionality in Twig templates Oct 10, 2024
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, we can continue from here later

@miguelvaara miguelvaara merged commit 2f51756 into main Oct 10, 2024
7 of 10 checks passed
@miguelvaara miguelvaara deleted the issue1523translate-messages-shown-by-vue-components-2 branch October 10, 2024 13:41
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.

Bring translations managed by Symfony into use in Vue templates
3 participants