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

[es, fr, ja, ko, pt-br, ru, zh-cn, zh-tw]: correct image file paths and demos paths #24765

Merged
merged 12 commits into from
Jan 21, 2025

Conversation

OHMORIYUSUKE
Copy link
Contributor

@OHMORIYUSUKE OHMORIYUSUKE commented Nov 25, 2024

Description

Issue #24373 has been addressed.
Other image files had also been changed. For example, starsolid.gif was renamed to star-solid.gif. Therefore, the file paths have been corrected.

This problem occurs in the documentation for each language. All have been corrected in this Pull Request.

Motivation

If images and demos are not displayed correctly, they will confuse people referring to the document.

Additional details

Related issues and pull requests

Fixes #24373

@OHMORIYUSUKE OHMORIYUSUKE requested review from a team as code owners November 25, 2024 18:16
@OHMORIYUSUKE OHMORIYUSUKE requested review from Jalkhov, leon-win, 1ilsang, mfuji09, cw118, JasonLamv-t and nathipg and removed request for a team November 25, 2024 18:16
@github-actions github-actions bot added l10n-ja Issues related to Japanese content. l10n-fr Issues related to French content. l10n-zh Issues related to Chinese content. l10n-es Issues related to Spanish content. l10n-ru Issues related to Russian content. l10n-ko Issues related to Korean content. l10n-pt-br Issues related to Brazilian Portuguese labels Nov 25, 2024
@Graywolf9 Graywolf9 requested review from Graywolf9 and removed request for Jalkhov November 26, 2024 22:12
Copy link
Contributor

@Graywolf9 Graywolf9 left a comment

Choose a reason for hiding this comment

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

The paths looks good for es

Thank you so much!

@OHMORIYUSUKE
Copy link
Contributor Author

@1ilsang @cw118 @nathipg
Could you kindly review this when you have a moment?

Copy link
Member

@cw118 cw118 left a comment

Choose a reason for hiding this comment

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

Most of the changes look good, thank you!

I have not had the time to test on my local machine, but could you confirm that the EmbedLiveSample work after adding the titles (e.g. "Exemple_simple")? There is a reason why empty strings were used in these macros in translated-content.

@1ilsang
Copy link
Member

1ilsang commented Dec 1, 2024

Looks good for ko. Thank you so much.

@OHMORIYUSUKE
Copy link
Contributor Author

@cw118

Most of the changes look good, thank you!

I have not had the time to test on my local machine, but could you confirm that the EmbedLiveSample work after adding the titles (e.g. "Exemple_simple")? There is a reason why empty strings were used in these macros in translated-content.

Thank you for the review!

The EmbedLiveSample works correctly. I confirmed it on my local machine.

There is a reason why empty strings were used in these macros in translated-content.

The display was not working as expected, so I fixed it in this pull request.
I referred to the following documentation when making the corrections:
https://developer.mozilla.org/en-US/docs/MDN/Writing_guidelines/Page_structures/Live_samples#grouping_code_blocks

I couldn't understand the reason why the macro was used with empty strings.My apologies.

Copy link
Member

@cw118 cw118 left a comment

Choose a reason for hiding this comment

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

The EmbedLiveSample works correctly. I confirmed it on my local machine.

Perfect, that should work then. We can confirm again once this is merged. Thanks very much for this @OHMORIYUSUKE !

In the past there were some strange issues with EmbedLiveSample in translated-content that required using empty strings, but it does seem that this is not a problem anymore (for the most part).

@OHMORIYUSUKE
Copy link
Contributor Author

@nathipg
Could you kindly review this when you have a moment?

@OHMORIYUSUKE
Copy link
Contributor Author

@josielrocha

You're not assigned as a reviewer, but if possible, could you kindly review it? Thank you in advance!

@cw118
Copy link
Member

cw118 commented Jan 9, 2025

Gently poking @mdn/yari-content-pt-br for a review :)

@leon-win
Copy link
Member

leon-win commented Jan 9, 2025

I think there are so few pt-BR-related changes in this PR that it is not necessary to wait for confirmation from the pt-BR team =)

I fixed the typo (made a suggestion).

@cw118
Copy link
Member

cw118 commented Jan 10, 2025

I think there are so few pt-BR-related changes in this PR that it is not necessary to wait for confirmation from the pt-BR team =)

Thank you for reviewing pt-br! I agree, especially since this PR has been open for a long time, though I personally still prefer to try to ping them because locales may have conventions that only they know, among other reasons.

Let's merge this once the typo is fixed (N.B. the same typo seems to be here as well) :)

@Graywolf9
Copy link
Contributor

Hello to every one!

It seems that the pt-br samples are rendering succesfully, also I can't find but I remember somethign about removing the first argument in EmbdeLiveSample macro, so maybe we can continue with this PR

I don't know what do you think

@cw118
Copy link
Member

cw118 commented Jan 21, 2025

Hello to every one!

It seems that the pt-br samples are rendering succesfully, also I can't find but I remember somethign about removing the first argument in EmbdeLiveSample macro, so maybe we can continue with this PR

I don't know what do you think

Yes, previously EmbedLiveSample behaved strangely and on translated-content we used empty strings in the macro, but this does not seem to be required anymore? I still have to catch up on some MDN updates so I may have missed it somewhere 😅

If the pt-br samples display well, then I think we can merge this now because it has been open for a long time. @leon-win maybe the typo that you found can be suggested/fixed in a separate PR (or an issue can be created) to avoid blocking this one? Happy to discuss more though!

@leon-win
Copy link
Member

Hello to every one!
It seems that the pt-br samples are rendering succesfully, also I can't find but I remember somethign about removing the first argument in EmbdeLiveSample macro, so maybe we can continue with this PR
I don't know what do you think

Yes, previously EmbedLiveSample behaved strangely and on translated-content we used empty strings in the macro, but this does not seem to be required anymore? I still have to catch up on some MDN updates so I may have missed it somewhere 😅

If the pt-br samples display well, then I think we can merge this now because it has been open for a long time. @leon-win maybe the typo that you found can be suggested/fixed in a separate PR (or an issue can be created) to avoid blocking this one? Happy to discuss more though!

Sorry if I got in the way. I wanted to help))
Closed my suggestion 👌

@cw118
Copy link
Member

cw118 commented Jan 21, 2025

Sorry if I got in the way. I wanted to help)) Closed my suggestion 👌

No, not at all, thank you for seeing that typo! I was just thinking we could perhaps try another issue or PR so that the pt-br team can confirm it is correct without keeping this PR open. Would you like to create the issue/PR? If not I would be glad to do it 😃

I will merge since this most locales are approved, and thank you @leon-win and @Graywolf9 for checking the pt-br files! Thank you for your patience @OHMORIYUSUKE 🎉

@cw118 cw118 merged commit 37b94f9 into mdn:main Jan 21, 2025
7 checks passed
@leon-win
Copy link
Member

No, not at all, thank you for seeing that typo! I was just thinking we could perhaps try another issue or PR so that the pt-br team can confirm it is correct without keeping this PR open. Would you like to create the issue/PR? If not I would be glad to do it 😃

I agree, it's better when each PR solves one specific problem. However, it doesn't always work out that way)

I think it's better for you to do it)) 🤝

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
l10n-es Issues related to Spanish content. l10n-fr Issues related to French content. l10n-ja Issues related to Japanese content. l10n-ko Issues related to Korean content. l10n-pt-br Issues related to Brazilian Portuguese l10n-ru Issues related to Russian content. l10n-zh Issues related to Chinese content.
Projects
Development

Successfully merging this pull request may close these issues.

list-style-image の例に画像が適用されていない
7 participants