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

fix(build): copy images that are only in default locale to l10n folder #7917

Merged
merged 11 commits into from
Jul 17, 2023
Merged

fix(build): copy images that are only in default locale to l10n folder #7917

merged 11 commits into from
Jul 17, 2023

Conversation

yin1999
Copy link
Member

@yin1999 yin1999 commented Jan 6, 2023 •
edited
Loading

Summary

Fixes: #5652

Problem

The images used in live sample ({{EmbedLiveSample}}) are not served (404) when they are not duplicated on mdn/translated-content.

⚠️ Blocked by: #7921

Solution

See: #5652 (comment)

Copy images to l10n built folders.

We have serve the image files correctly in yarn-server, so we should only copy the images when in static build mode.

Screenshots

See testing.

How did you test this change?

run yarn test:testing.

yin1999
yin1999 commented Jan 6, 2023
View reviewed changes
testing/tests/index.test.ts Outdated Show resolved Hide resolved
@yin1999 yin1999 changed the title fix(build): copy images that are in default locale only to l10n folder fix(build): copy images that are only in default locale to l10n folder Jan 6, 2023
@github-actions github-actions bot added the merge conflicts 🚧 Please rebase onto or merge the latest main. label Jan 23, 2023
@github-actions
Copy link
Contributor

This pull request has merge conflicts that must be resolved before it can be merged.

@github-actions github-actions bot removed the merge conflicts 🚧 Please rebase onto or merge the latest main. label Jan 23, 2023
@yin1999 yin1999 mentioned this pull request Feb 8, 2023
Merged
@github-actions github-actions bot added the merge conflicts 🚧 Please rebase onto or merge the latest main. label Feb 8, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Feb 8, 2023

This pull request has merge conflicts that must be resolved before it can be merged.

@github-actions github-actions bot removed the merge conflicts 🚧 Please rebase onto or merge the latest main. label Feb 16, 2023
@yin1999 yin1999 marked this pull request as ready for review February 16, 2023 14:08
@yin1999
Copy link
Member Author

yin1999 commented Mar 10, 2023

@caugner, may I get a review :)

@github-actions github-actions bot added merge conflicts 🚧 Please rebase onto or merge the latest main. labels May 3, 2023
@github-actions
Copy link
Contributor

github-actions bot commented May 3, 2023

This pull request has merge conflicts that must be resolved before it can be merged.

1 similar comment
@github-actions
Copy link
Contributor

github-actions bot commented May 3, 2023

This pull request has merge conflicts that must be resolved before it can be merged.

@github-actions github-actions bot removed the merge conflicts 🚧 Please rebase onto or merge the latest main. label May 6, 2023
@github-actions github-actions bot added the merge conflicts 🚧 Please rebase onto or merge the latest main. label May 10, 2023
@github-actions
Copy link
Contributor

This pull request has merge conflicts that must be resolved before it can be merged.

@caugner
Copy link
Contributor

caugner commented May 11, 2023

@LeoMcA I would suggest that we handle this instead in our Cloud Function here:

https://github.com/mdn/yari/blob/f18d609c6ddfda87efca707234f9d4a6c49798df/cloud-function/src/handlers/proxy-content.ts#L29C1-L35

If we don't find an asset in /fr/..., try it again with /en-us/.

@yin1999
Copy link
Member Author

yin1999 commented May 14, 2023

We may also need to consider mdn/translated-content#7648

@caugner caugner self-assigned this Jul 17, 2023
@github-actions github-actions bot removed the merge conflicts 🚧 Please rebase onto or merge the latest main. label Jul 17, 2023
@caugner caugner added the tx localizer/translator experience label Jul 17, 2023
@caugner caugner self-requested a review July 17, 2023 13:14
caugner
caugner requested changes Jul 17, 2023
View reviewed changes
Copy link
Contributor

@caugner caugner left a comment

Choose a reason for hiding this comment

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

LGTM and tested locally by comparing full builds of this branch vs main, just two nits (one about naming the map variable, and another about reusing findByURLWithFallback). 🚀

build/check-images.ts Outdated Show resolved Hide resolved
build/check-images.ts Outdated Show resolved Hide resolved
build/index.ts Outdated Show resolved Hide resolved
build/check-images.ts Outdated Show resolved Hide resolved
build/cli.ts Outdated Show resolved Hide resolved
@yin1999 @caugner
Apply suggestions from code review
1da8c43 
Co-authored-by: Claas Augner <495429+caugner@users.noreply.github.com>
@yin1999
Copy link
Member Author

yin1999 commented Jul 17, 2023

Thanks for the suggestions @caugner. I've pushed 1da8c43 to resolve them :)

@yin1999 yin1999 requested a review from caugner July 17, 2023 15:20
caugner
caugner approved these changes Jul 17, 2023
View reviewed changes
Copy link
Contributor

@caugner caugner left a comment

Choose a reason for hiding this comment

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

LGTM, thanks a lot! 🚀

@caugner caugner merged commit 86a4e6f into mdn:main Jul 17, 2023
9 checks passed
@yin1999 yin1999 deleted the fix-5652 branch July 17, 2023 15:37
@github-actions github-actions bot mentioned this pull request Jul 17, 2023
Merged
@caugner
Copy link
Contributor

caugner commented Jul 17, 2023

Thank you @yin1999. I triggered an xyz-build, and once that has finished, you should be able to see the results here.

If all goes well, the images that are missing here on prod, should appear here on xyz.

@yin1999
Copy link
Member Author

yin1999 commented Jul 17, 2023

Thank you @yin1999. I triggered an xyz-build, and once that has finished, you should be able to see the results here.

If all goes well, the images that are missing here on prod, should appear here on xyz.

I'll keep an eye on the build results. Thanks for your advice as well :)

@yin1999
Copy link
Member Author

yin1999 commented Jul 17, 2023

Hi @caugner. I could actually see the image in 'zh-CN'. But the live sample in xyz build seems to be broken. I just use my phone to view this page, so I couldn't give more information (It's late at night in China, and I'm going to 🛏️). I think it's your time to fix this ;)

@caugner
Copy link
Contributor

caugner commented Jul 17, 2023

(The live samples are broken on xyz probably because we didn't update the workflow when the playground landed.)

This was referenced Jul 18, 2023
Closed
Closed
Closed
Merged
Closed
Closed
Closed
@yin1999 yin1999 mentioned this pull request Dec 2, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@caugner caugner caugner approved these changes

Assignees

@caugner caugner

Labels
tx localizer/translator experience
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Live samples in translated-content cannot use images from content
2 participants
@yin1999 @caugner