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

downloading external images for translated-content #3207

Merged
merged 2 commits into from
Mar 12, 2021
Merged

downloading external images for translated-content #3207

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
871261b
downloading external images for translated-content
peterbe Mar 11, 2021
525abc6
eslint
peterbe Mar 11, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 22 additions & 6 deletions build/check-images.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ const sizeOf = require("image-size");
const { Document, Image } = require("../content");
const { FLAW_LEVELS } = require("./constants");
const { findMatchesInText } = require("./matches-in-text");
const { DEFAULT_LOCALE } = require("../libs/constants");

/**
* Mutate the `$` instance for image reference and if appropriate,
Expand Down Expand Up @@ -119,9 +120,24 @@ function checkImageReferences(doc, $, options, { url, rawContent }) {
// it now, we still want the full relative URL.
img.attr("src", absoluteURL.pathname);
} else {
let suggestion = null;
// If this document is *not* en-US, perhaps the external image has already
// been downloaded by the en-US equivalent. If so, make that the suggestion.
if (doc.locale !== DEFAULT_LOCALE) {
const filePath = Image.findByURL(
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 🤷

);
if (filePath) {
suggestion = path.basename(filePath);
}
}
addImageFlaw(img, src, {
explanation: "External image URL",
externalImage: true,
suggestion,
});
}
}
Expand All @@ -137,16 +153,16 @@ function checkImageReferences(doc, $, options, { url, rawContent }) {
let enUSFallback = false;
if (
!filePath &&
doc.locale !== "en-US" &&
!finalSrc.startsWith("/en-us/")
doc.locale !== DEFAULT_LOCALE &&
!finalSrc.startsWith(`/${DEFAULT_LOCALE.toLowerCase()}/`)
) {
const enFinalSrc = finalSrc.replace(
const enUSFinalSrc = finalSrc.replace(
`/${doc.locale.toLowerCase()}/`,
"/en-us/"
`/${DEFAULT_LOCALE.toLowerCase()}/`
);
if (Image.findByURL(enFinalSrc)) {
if (Image.findByURL(enUSFinalSrc)) {
// 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.

// Note that this `<img src="...">` value can work if you use the
// en-US equivalent URL instead.
enUSFallback = true;
Expand Down
7 changes: 4 additions & 3 deletions build/flaws.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ const {
} = require("./matches-in-text");
const { humanFileSize } = require("./utils");
const { VALID_MIME_TYPES } = require("../filecheck/constants");
const { DEFAULT_LOCALE } = require("../libs/constants");

function injectFlaws(doc, $, options, { rawContent }) {
if (doc.isArchive) return;
Expand Down Expand Up @@ -574,7 +575,9 @@ async function fixFixableFlaws(doc, options, document) {
// HTML. It's only proper HTML when the kumascript macros have been
// expanded.
let newSrc;
if (flaw.externalImage) {
if (flaw.suggestion) {
newSrc = flaw.suggestion;
} else {
// Sanity check that it's an external image
const url = new URL(forceExternalURL(flaw.src));
if (url.protocol !== "https:") {
Expand Down Expand Up @@ -673,8 +676,6 @@ async function fixFixableFlaws(doc, options, document) {
throw error;
}
}
} else {
newSrc = flaw.suggestion;
}
newRawHTML = replaceMatchesInText(flaw.src, newRawHTML, newSrc, {
inAttribute: "src",
Expand Down