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.

Sign up for GitHub

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

Jump to bottom

[api-minor] Use "data-l10n-id"/"data-l10n-args", rather than manually updating DOM-elements, to trigger translation (PR 17146 follow-up) #17141

Merged
merged 1 commit into from
Oct 23, 2023
Merged

[api-minor] Use "data-l10n-id"/"data-l10n-args", rather than manually updating DOM-elements, to trigger translation (PR 17146 follow-up) #17141

merged 1 commit into from
Oct 23, 2023

Conversation

Snuffleupagus
Copy link
Collaborator

@Snuffleupagus Snuffleupagus commented Oct 19, 2023 •
edited

This patch changes almost all viewer-components[1] to use "data-l10n-id"/"data-l10n-args" for localization, which means that in many cases we no longer need to pass around the L10n-instance any more.

One part of the code-base where the L10n-instance is still being used "directly" is the AnnotationEditors, however while it might be possible to convert (most of) that code as well that's not attempted in this patch.

[1] The one exception is the PDFDocumentProperties dialog, since the way it's currently implemented makes that less straightforward to fix without a lot of code changes.

@Snuffleupagus
Copy link
Collaborator Author

@calixteman Does this make sense to do, as a general pattern, where possible in the viewer?
Since I wasn't sure if this is actually desirable, I've so far only converted one file in this patch.

@calixteman
Copy link
Contributor

Yep it makes sense as a general pattern: it makes the code simpler.

@Snuffleupagus Snuffleupagus force-pushed the l10n-more-setAttribute branch 3 times, most recently from 72e9b5d to b6955ac Compare October 19, 2023 16:43
@calixteman calixteman added the l10n Localization label Oct 19, 2023
@Snuffleupagus Snuffleupagus added the release-blocker Blocker for the upcoming release label Oct 19, 2023
@Snuffleupagus Snuffleupagus changed the title Use "data-l10n-id"/"data-l10n-args" more, rather than manually updating DOM-elements to trigger translation (PR 17115 follow-up) Use "data-l10n-id"/"data-l10n-args" more, rather than manually updating DOM-elements to trigger translation (PR 17146 follow-up) Oct 21, 2023
@Snuffleupagus Snuffleupagus force-pushed the l10n-more-setAttribute branch 4 times, most recently from e453e16 to c5ecdd7 Compare October 21, 2023 16:39
@Snuffleupagus Snuffleupagus force-pushed the l10n-more-setAttribute branch 2 times, most recently from 5ee9225 to a458574 Compare October 21, 2023 16:52
@Snuffleupagus
Copy link
Collaborator Author

/botio test

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Windows)

Received

Command cmd_test from @Snuffleupagus received. Current queue size: 0

Live output at: http://54.193.163.58:8877/3d4f7233987275c/output.txt

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)

Received

Command cmd_test from @Snuffleupagus received. Current queue size: 0

Live output at: http://54.241.84.105:8877/118643ab5050634/output.txt

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)

Failed

Full output at http://54.241.84.105:8877/118643ab5050634/output.txt

Total script time: 24.82 mins

  • Font tests: Passed
  • Unit tests: Passed
  • Integration Tests: Passed
  • Regression tests: FAILED
  different ref/snapshot: 15
  different first/second rendering: 2

Image differences available at: http://54.241.84.105:8877/118643ab5050634/reftest-analyzer.html#web=eq.log

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Windows)

Failed

Full output at http://54.193.163.58:8877/3d4f7233987275c/output.txt

Total script time: 36.01 mins

  • Font tests: FAILED
  • Unit tests: Passed
  • Integration Tests: Passed
  • Regression tests: FAILED
  different ref/snapshot: 23

Image differences available at: http://54.193.163.58:8877/3d4f7233987275c/reftest-analyzer.html#web=eq.log

@Snuffleupagus Snuffleupagus changed the title Use "data-l10n-id"/"data-l10n-args" more, rather than manually updating DOM-elements to trigger translation (PR 17146 follow-up) Use "data-l10n-id"/"data-l10n-args", rather than manually updating DOM-elements, to trigger translation (PR 17146 follow-up) Oct 22, 2023
@Snuffleupagus Snuffleupagus changed the title Use "data-l10n-id"/"data-l10n-args", rather than manually updating DOM-elements, to trigger translation (PR 17146 follow-up) [api-minor] Use "data-l10n-id"/"data-l10n-args", rather than manually updating DOM-elements, to trigger translation (PR 17146 follow-up) Oct 22, 2023
@Snuffleupagus Snuffleupagus marked this pull request as ready for review October 22, 2023 09:03
@Snuffleupagus Snuffleupagus requested a review from a team as a code owner October 22, 2023 09:03
flodolo
flodolo approved these changes Oct 22, 2023
View reviewed changes
Copy link
Contributor

@flodolo flodolo left a comment

Choose a reason for hiding this comment

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

Note that this impacts the migration in https://phabricator.services.mozilla.com/D190941

@Snuffleupagus
Copy link
Collaborator Author

It might be a completely pointless micro-optimization, but one thought that occurred to me is that we could probably build the l10n-args "manually" using template strings and thus avoid the JSON.stringify-calls.

timvandermeij
timvandermeij approved these changes Oct 22, 2023
View reviewed changes
Copy link
Contributor

@timvandermeij timvandermeij left a comment •
edited

Choose a reason for hiding this comment

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

Looks good to me, with the comment below addressed, but I'd like @calixteman to also sign off on this given that the Fluent migration also needs to be adapted. I think it's great that we can avoid all parameter passing here now!

test/driver.js Show resolved Hide resolved
@Snuffleupagus
[api-minor] Use "data-l10n-id"/"data-l10n-args", rather than manually…
17af706 
… updating DOM-elements, to trigger translation (PR 17146 follow-up)

This patch changes almost all viewer-components[1] to use "data-l10n-id"/"data-l10n-args" for localization, which means that in many cases we no longer need to pass around the `L10n`-instance any more.

One part of the code-base where the `L10n`-instance is still being used "directly" is the AnnotationEditors, however while it might be possible to convert (most of) that code as well that's not attempted in this patch.

---
[1] The one exception is the `PDFDocumentProperties` dialog, since the way it's currently implemented makes that less straightforward to fix without a lot of code changes.
calixteman
calixteman approved these changes Oct 23, 2023
View reviewed changes
Copy link
Contributor

@calixteman calixteman left a comment

Choose a reason for hiding this comment

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

LGTM.
I'll update the migration recipe in m-c to take into account the new changes here.

@Snuffleupagus Snuffleupagus merged commit 8376b3f into mozilla:master Oct 23, 2023
3 checks passed
@Snuffleupagus Snuffleupagus deleted the l10n-more-setAttribute branch October 23, 2023 10:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@timvandermeij timvandermeij timvandermeij approved these changes

@flodolo flodolo flodolo approved these changes

@calixteman calixteman calixteman approved these changes

Assignees
No one assigned
Labels
core l10n Localization release-blocker Blocker for the upcoming release test viewer
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@Snuffleupagus @calixteman @moz-tools-bot @timvandermeij @flodolo