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

downloading external images for translated-content #3207

Conversation

peterbe
Copy link
Contributor

@peterbe peterbe commented Mar 11, 2021

Fixes #3174

Seems to work! I was able to create mdn/translated-content#110 with this change.
And just to check that then en-US fixable external image flaws still work, I was able to create mdn/content#3040
It almost seemed too good to be true.

The assumption this is based on is that the basename of the external image URL would match perfectly the en-US equivalent.
I think it's a safe assumption. It COULD be that someone has a "localized image" which they uploaded under a different ID but with the exact same basename. I find that unlikely enough to not care.

I've noticed that in fr there are a lot of images that are genuinely different. For example choix-firefox.png was one I found. In these cases, if a fr translator downloads it, it'll be fine. It'll simply be a different binary image.

path.join(
doc.mdn_url.replace(`/${doc.locale}/`, `/${DEFAULT_LOCALE}/`),
path.basename(src)
)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is this really the best way to "hop over" to the en-US content? It certainly seems to work :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Best way for now 🤷

// Use the en-US src instead
finalSrc = enFinalSrc;
finalSrc = enUSFinalSrc;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The above 6 lines of changes are just tidying up (refactoring).
Instead of hardcoding the string "en-us" it now uses DEFAULT_LOCALE.
And if the first variable is called enUSFallback the other one should be called enUSFinalSrc to match the prefixes.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm always scared when I see how fragile our locale casing is.

@peterbe peterbe requested a review from fiji-flo March 11, 2021 22:23
path.join(
doc.mdn_url.replace(`/${doc.locale}/`, `/${DEFAULT_LOCALE}/`),
path.basename(src)
)
Copy link
Contributor

Choose a reason for hiding this comment

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

Best way for now 🤷

// Use the en-US src instead
finalSrc = enFinalSrc;
finalSrc = enUSFinalSrc;
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm always scared when I see how fragile our locale casing is.

@fiji-flo fiji-flo merged commit 0b000dc into mdn:main Mar 12, 2021
@peterbe peterbe deleted the 3174-downloading-external-images-for-translated-content branch March 12, 2021 16:55
wbamberg pushed a commit to wbamberg/yari that referenced this pull request Mar 15, 2021
* upstream/main: (164 commits)
  skip empty srcs for safe iframe srcs (mdn#3216)
  correct code comment (mdn#3223)
  build(deps): bump ahmadnassri/action-dependabot-auto-merge (mdn#3197)
  build(deps-dev): bump @types/react-dom from 17.0.1 to 17.0.2 (mdn#3164)
  create a whatsdeployed.json for translated-content too (mdn#3221)
  avoid double-slash redirects (mdn#3222)
  build(deps): bump image-size from 0.9.4 to 0.9.5 (mdn#3214)
  build(deps): bump boto3 from 1.17.22 to 1.17.26 in /deployer (mdn#3212)
  Fix our auto-merge workflow (mdn#3218)
  build(deps-dev): bump ts-loader from 8.0.17 to 8.0.18 (mdn#3208)
  disable lighthouse PR check unless relevant changes (mdn#3203)
  hide toolbar for frozen locales (mdn#3213)
  build(deps): bump is-svg from 4.2.1 to 4.2.2 (mdn#3209)
  build(deps): bump @mdn/browser-compat-data from 3.1.3 to 3.2.0 (mdn#3210)
  downloading external images for translated-content (mdn#3207)
  add active locales (mdn#3201)
  add tool command for rendering/removing macros (mdn#2955)
  unsafe html should be a breaking flaw (mdn#3192)
  open editor for translated content (mdn#3196)
  add fundamental redirects for /en-US/Security/CSP (mdn#3200)
  ...
peterbe added a commit to peterbe/yari that referenced this pull request Jun 1, 2021
* downloading external images for translated-content

Fixes mdn#3174

* eslint
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.

Image flaw checking should be different for translated content
2 participants