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

Amp-story translations #35333

Merged
merged 20 commits into from
Sep 14, 2021
Merged

Amp-story translations #35333

merged 20 commits into from
Sep 14, 2021

Conversation

ilyaspiridonov
Copy link
Contributor

Updates translations in .json files for amp-story UI labels (69 languages)
Translations by Alconost

@amp-owners-bot amp-owners-bot bot requested a review from newmuis July 21, 2021 15:34
@amp-owners-bot
Copy link

amp-owners-bot bot commented Jul 21, 2021

Hey @gmajoulet, @newmuis! These files were changed:

extensions/amp-story/1.0/_locales/af.json
extensions/amp-story/1.0/_locales/am.json
extensions/amp-story/1.0/_locales/ar.json
extensions/amp-story/1.0/_locales/bg.json
extensions/amp-story/1.0/_locales/bn.json
extensions/amp-story/1.0/_locales/bs.json
extensions/amp-story/1.0/_locales/ca.json
extensions/amp-story/1.0/_locales/cs.json
extensions/amp-story/1.0/_locales/da.json
extensions/amp-story/1.0/_locales/de.json
extensions/amp-story/1.0/_locales/default.json
extensions/amp-story/1.0/_locales/el.json
+59 more

@newmuis
Copy link
Contributor

newmuis commented Jul 28, 2021

@danielrozenberg do you know why the bundle-size check is still pending even after several days? Given the number of lines changed here, we'd want to check that before approving

@danielrozenberg
Copy link
Member

@danielrozenberg do you know why the bundle-size check is still pending even after several days? Given the number of lines changed here, we'd want to check that before approving

Looks like the bundle-size job was cancelled because of a failure in unit tests - I recommend rebasing on main and force-pushing. We just removed the fail-fast behaviour, so even if there are failures in other jobs, the bundle-size job will continue

@newmuis
Copy link
Contributor

newmuis commented Jul 28, 2021

Thanks @danielrozenberg!

@ilyaspiridonov do you mind rebasing and force pushing as Daniel mentions above? Thanks!

@ilyaspiridonov
Copy link
Contributor Author

@newmuis any idea why circleCI is failing?

@newmuis
Copy link
Contributor

newmuis commented Aug 3, 2021

@calebcordry do you know (since it's an amp-story-auto-ads E2E test that is failing)?

@gmajoulet
Copy link
Contributor

I restarted the test as it looks like a flake. That should do it.

The bundle-size saw a 5.37KB increase which is rather large. Can we look into easy options to make this better?
The first idea that comes to mind would be removing the "description" from non en.json files as it's not used. This should be very impactful. I'll let @newmuis suggest a way forward here.

@newmuis
Copy link
Contributor

newmuis commented Aug 4, 2021

Agreed that removing descriptions from every file except for en.json would be helpful.

@ilyaspiridonov would you mind doing that so we can re-evaluate the bundle size?

@newmuis
Copy link
Contributor

newmuis commented Aug 16, 2021

Ping @ilyaspiridonov, do you mind removing the descriptions from the non-English translations for now?

@ilyaspiridonov
Copy link
Contributor Author

@newmuis Apologies for the long wait (PTO and traveling) - should be good now, but let's see what the bundle-size outputs.

@gmajoulet
Copy link
Contributor

gmajoulet commented Aug 31, 2021

Could you please add a new line at the end of each file? It looks like our systems are complaining about this.
Thanks!

@processprocess
Copy link
Contributor

We deprecated a feature recently and will need to remove these 4 i18n strings from these files:
AMP_STORY_WARNING_DESKTOP_HEIGHT_SIZE_TEXT: '37',
AMP_STORY_WARNING_DESKTOP_SIZE_TEXT: '18',
AMP_STORY_WARNING_DESKTOP_WIDTH_SIZE_TEXT: '38',
AMP_STORY_WARNING_LANDSCAPE_ORIENTATION_TEXT: '20',

Is it possible to make this change in this PR?

@ilyaspiridonov
Copy link
Contributor Author

We deprecated a feature recently and will need to remove these 4 i18n strings from these files:
AMP_STORY_WARNING_DESKTOP_HEIGHT_SIZE_TEXT: '37',
AMP_STORY_WARNING_DESKTOP_SIZE_TEXT: '18',
AMP_STORY_WARNING_DESKTOP_WIDTH_SIZE_TEXT: '38',
AMP_STORY_WARNING_LANDSCAPE_ORIENTATION_TEXT: '20',

Is it possible to make this change in this PR?

I don't think they are in these files, am I missing something? @processprocess

@processprocess
Copy link
Contributor

AMP_STORY_WARNING_DESKTOP_HEIGHT_SIZE_TEXT: '37',
AMP_STORY_WARNING_DESKTOP_SIZE_TEXT: '18',
AMP_STORY_WARNING_DESKTOP_WIDTH_SIZE_TEXT: '38',
AMP_STORY_WARNING_LANDSCAPE_ORIENTATION_TEXT: '20',
Is it possible to make this change in this PR?

I don't think they are in these files, am I missing something? @processprocess

My bad, to clarify:
We need the strings keyed to 37, 18, 38 and 20 removed in each file.

@newmuis
Copy link
Contributor

newmuis commented Sep 8, 2021

@ilyaspiridonov are you able to provide translations for Portuguese (as spoken in Portugal, rather than Brazil) and Slovak?

The PR is currently failing because these files are expected to exist. If we don't want them to exist, we'll need to deprecate them, which will be a whole separate process.

Thanks for following through with this! Getting the first PR through will be the hardest; once we've established the right structure, hopefully we can continue to replicate it for future batches.


@processprocess let's remove these in a followup PR to make this one easier to get through

@processprocess
Copy link
Contributor

@processprocess let's remove these in a followup PR to make this one easier to get through

👍 Filed #35934 for tracking.

@newmuis
Copy link
Contributor

newmuis commented Sep 10, 2021

Thanks for the updates @ilyaspiridonov!

Looks like it's almost there, but the new files reintroduced formatting issues; do you mind updating those as well?

@ilyaspiridonov
Copy link
Contributor Author

@newmuis Thank for the heads-up, the checks seem to be green now!

@newmuis
Copy link
Contributor

newmuis commented Sep 13, 2021

Thanks for working through this! Looks like it's passing the code checks now, and we can calculate the bundle size changes:

dist/v0/amp-story-0.1.mjs: Δ +3.12KB
dist/v0/amp-story-1.0.mjs: Δ +3.12KB
dist/v0/amp-story-0.1.js: Δ +3.03KB
dist/v0/amp-story-1.0.js: Δ +3.03KB

/to @gmajoulet for analysis on whether that increase is reasonable.

@gmajoulet
Copy link
Contributor

A few things to feel better about adding these extra 3KB:

  • our users actually need these
  • we just removed 2kB here https://github.com/ampproject/amphtml/runs/3558679416
  • lazy loading the translation strings isn't straightforward as some are required for the initial render
  • there are easier opportunities for impact: some huge chunks of code could easily be lazy loaded (sharing, page attachments, etc)

@gmajoulet
Copy link
Contributor

cc @ampproject/wg-performance FYI, I'll wait a bit in case you want to chime in before merging

@samouri
Copy link
Member

samouri commented Sep 14, 2021

@gmajoulet: FMI Is the current status quo to load all languages instead of just one? have we done any research to determine the size savings from only loading 1?

@gmajoulet
Copy link
Contributor

Loading just one today would mean either:

  1. Lazy loading from the JS, hurting LCP and UX
  2. Write an AMP transformer to add a tag to load the correct language, which would have my vote but it's work we haven't prioritized. I can track it somewhere

@samouri
Copy link
Member

samouri commented Sep 14, 2021

Have we done research to determine the size savings from only loading 1? That would help determine priority.

@gmajoulet
Copy link
Contributor

They're 3+KBs out of 70 so it'd be a 5-10% reduction in bundle size.

@gmajoulet gmajoulet merged commit 7d82fa1 into ampproject:main Sep 14, 2021
@gmajoulet gmajoulet added this to In progress in wg-stories Sprint via automation Sep 14, 2021
rbeckthomas pushed a commit to rbeckthomas/amphtml that referenced this pull request Sep 14, 2021
* Updated translations

* Remove comments from translated files

* add new line at the end of the localization files

* add et, pt-PT and sl translations to amp-story

* remove lines 37, 38, 18, 20 from amp-story i18n files

* fix spaces

* fix spaces in loc files

* formatting fixes in translation files

* formatting fixes in translation files

* fix formatting in translation files

* fox some more formatting issues in translation files

* more fixes in translation files
@newmuis
Copy link
Contributor

newmuis commented Sep 15, 2021

+1 in my opinion not a reason to block this current work, but also +1 to having some sort of tracking for a "real" fix of how to only serve the language the user needs. The 3 KB is the diff from what we already have, but I'd expect we'd shave more than 3 KB if we could find a way to only serve one language without the PX hit.

(A complex strawman: separately define render-blocking strings vs. non-render-blocking strings. Only bundle the render-blocking ones, late-load the rest, which AFAIK would/could be the majority). A transformer could do a lot (e.g. we know the quiz/poll strings are render-blocking if there is a quiz/poll on the first page of the story), but there may also be diminishing returns here.


Separate from any particular proposal, @gmajoulet do you mind filing a tracking issue for finding high-level alignment on a solution here?

@gmajoulet
Copy link
Contributor

I'm tracking various performance improvement project that I'm trying to sort by category (LCP/bundle-size/runtime performance etc). I'll finalize all of that and track it publicly as part of a larger bundle-size project.

@rcebulko
Copy link
Contributor

rcebulko commented Sep 15, 2021

@samouri translation/localization would be very SSR-able with potential to avoid any render-blocking caused by waiting for the strings to load.

Simple option is having a split out critical-path-strings.js etc that's much smaller, inlined, w/e

caroqliu pushed a commit to caroqliu/amphtml that referenced this pull request Sep 16, 2021
* Updated translations

* Remove comments from translated files

* add new line at the end of the localization files

* add et, pt-PT and sl translations to amp-story

* remove lines 37, 38, 18, 20 from amp-story i18n files

* fix spaces

* fix spaces in loc files

* formatting fixes in translation files

* formatting fixes in translation files

* fix formatting in translation files

* fox some more formatting issues in translation files

* more fixes in translation files
@processprocess processprocess moved this from In progress to Done in wg-stories Sprint Sep 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

None yet

8 participants