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

fr: Removes duplicated frontmatter keys (learn folder) #13663

Merged
merged 6 commits into from
Jul 20, 2023

Conversation

Lou8is
Copy link
Contributor

@Lou8is Lou8is commented Jun 10, 2023

Description

Removes duplicated frontmatter keys (see #7412) from the files/fr/learn sub-folder.

Only title, short_title, slug and l10n.* are left, essentially removed tags, original_slug, browser-compat, file-type.

Motivation

Help keeping the FR translation nice and up-to-date :)

Additional details

Related issues and pull requests

Relates to #7412
Relates to 6 other PR for the other subfolders (#13629, #13630, #13631, #13632, #13633, #13664)

@github-actions github-actions bot added the l10n-fr Issues related to French content. label Jun 10, 2023
Copy link
Member

@SphinxKnight SphinxKnight left a comment

Choose a reason for hiding this comment

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

multi-line metadata should be properly removed.
As far as I can see (I searched for : >-):

  • /files/fr/learn/forms/how_to_build_custom_form_controls/index.md
  • /files/fr/learn/forms/how_to_build_custom_form_controls/example_1/index.md
  • /files/fr/learn/forms/how_to_build_custom_form_controls/example_2/index.md
  • /files/fr/learn/forms/how_to_build_custom_form_controls/example_3/index.md
  • /files/fr/learn/forms/how_to_build_custom_form_controls/example_4/index.md
  • /files/fr/learn/forms/how_to_build_custom_form_controls/example_5/index.md
  • /files/fr/learn/javascript/objects/adding_bouncing_balls_features/index.md

need to be edited.

@SphinxKnight
Copy link
Member

@Lou8is thanks that was fast :) (I was working on it locally as well)

@Lou8is
Copy link
Contributor Author

Lou8is commented Jul 3, 2023

I had time and saw the notification so... 😄
However I am not sure how to handle the PR Test fail? I do not understand whats wrong (I'd guess the {{LearnSidebar}} macro).

@SphinxKnight
Copy link
Member

I had time and saw the notification so... 😄 However I am not sure how to handle the PR Test fail? I do not understand whats wrong (I'd guess the {{LearnSidebar}} macro).

thanks for this (and for all the work on this PR and the others)

I think it has to do with the content used from the subpages in the EmbedLiveSample calls. I'm investigating and comparing with en-US to be sure.

@SphinxKnight
Copy link
Member

I'll be back at it later tonight or tomorrow :)

@SphinxKnight SphinxKnight self-assigned this Jul 3, 2023
@Lou8is
Copy link
Contributor Author

Lou8is commented Jul 6, 2023

Well removing EmbedLiveSample macros from /fr/docs/Learn/Forms/How_to_build_custom_form_controls also removes the errors so you were right. However, i compared that with the EN content, and it does not seems to be any difference regarding that.

I suspect the error comes from arguments being deprecated in the EmbedLiveSample macro (see line 17 of https://github.com/mdn/yari/blob/main/kumascript/macros/EmbedLiveSample.ejs).

That might be a whole other issue to rewrite pages so they don't call out-of-page code samples.

@cw118
Copy link
Member

cw118 commented Jul 6, 2023

It seems that the EmbedLiveSample macros on the en-US page need to be updated as well. I opened an issue on mdn/content to confirm and I am preparing a pull request (for en-US).

@cw118
Copy link
Member

cw118 commented Jul 7, 2023

The accepted solution on en-US (cf. mdn/content#27813) was to copy the code of each EmbedLiveSample from the Learn/Forms/How_to_build_custom_form_controls/Example_1, Learn/Forms/How_to_build_custom_form_controls/Example_2 etc. pages and add it to the Learn/Forms/How_to_build_custom_form_controls file above each corresponding EmbedLiveSample using hidden codeblocks (html hidden, css hidden. js hidden).

Some comments (Styles requis, etc.) were removed from the hidden codeblocks (not from the Learn/Forms/How_to_build_custom_form_controls/Example pages!), and links pointing to the source code need to be moved as well.

See the pull request diff on mdn/content for more details, and let us know if help is needed for fixing the live samples :)

@Lou8is
Copy link
Contributor Author

Lou8is commented Jul 8, 2023

I've fixed that page as you demonstrated and did not thought about looking into the other pages.

Seems like these files are also impacted, at list in the FR translation. I'm working on them but listing them if you'd like to check the EN content.

(Impacted macros per page)

  • (1) /fr/learn/forms/how_to_structure_a_web_form/index.md
  • (1) /fr/learn/forms/your_first_form/index.md
  • (3) /fr/learn/html/introduction_to_html/advanced_text_formatting/index.md
  • (7) /fr/learn/html/introduction_to_html/getting_started/index.md
  • (2) /fr/learn/html/multimedia_and_embedding/images_in_html/index.md
  • (1) /fr/learn/html/multimedia_and_embedding/other_embedding_technologies/index.md
  • (1) /fr/learn/html/tables/advanced/index.md
  • (2) /fr/learn/javascript/building_blocks/conditionals/index.md
  • (2) /fr/learn/javascript/building_blocks/events/index.md
  • (2) /fr/learn/javascript/first_steps/a_first_splash/index.md

@Lou8is
Copy link
Contributor Author

Lou8is commented Jul 8, 2023

Some macros use "hide-codepen-jsfiddle" as parameter $6. Removing it does not seem to have functional impact, however it changes the page display (more margins or something). Any advice on that?

Edit: checks passed, maybe I did not modified those files. I could modified them in this PR or in a separate one.

@cw118
Copy link
Member

cw118 commented Jul 16, 2023

Some macros use "hide-codepen-jsfiddle" as parameter $6. Removing it does not seem to have functional impact, however it changes the page display (more margins or something). Any advice on that?

Parameter $6 is also deprecated. However, some say that it was incorrectly/prematurely deprecated, so MDN might keep this parameter.

In case MDN does keep parameter $6 (perhaps also parameter $5) in the future, I would say if the en-US page uses it and all checks pass then we can keep the parameter. Not 100% sure though.

@SphinxKnight any thoughts regarding parameters $5 and $6 of EmbedLiveSample?

@cw118
Copy link
Member

cw118 commented Jul 16, 2023

Edit: checks passed, maybe I did not modified those files. I could modified them in this PR or in a separate one.

Since this PR already modifies a lot of files/lines and the scope is not to fix EmbedLiveSample macros (and clarification is needed regarding parameters $5 and $6), let's remove the remaining deprecated EmbedLiveSample parameters in a separate one.

@SphinxKnight
Copy link
Member

Edit: checks passed, maybe I did not modified those files. I could modified them in this PR or in a separate one.

Since this PR already modifies a lot of files/lines and the scope is not to fix EmbedLiveSample macros (and clarification is needed regarding parameters $5 and $6), let's remove the remaining deprecated EmbedLiveSample parameters in a separate one.

Agreed :)
Sorry for the late reaction here, @Lou8is please let us know if you need help spltting the EmbedLiveSample work on another PR.

@github-actions github-actions bot added l10n-es Issues related to Spanish content. l10n-ru Issues related to Russian content. l10n-ko Issues related to Korean content. system Infrastructure and configuration for the project labels Jul 19, 2023
@Lou8is
Copy link
Contributor Author

Lou8is commented Jul 19, 2023

@SphinxKnight hmm not convinced I've managed this one correctly, I might need some help on reverting (or I could just re-do the edits on a clean branch).

  • I've reverted only up to a851416 as it was corrections to the original modifications.

@SphinxKnight
Copy link
Member

SphinxKnight commented Jul 19, 2023

@Lou8is if you made sure the "newer commits" were on another branch for #14401 , there's no need to create another branch, locally (assuming you have #7412-cleans-metadata-learn as a branch and origin as a remote to your GH fork), you can:

git checkout #7412-cleans-metadata-learn 
git reset --hard a8514164d3f252eaf83eb9b74436ba8c3c80ca43
git push -f origin #7412-cleans-metadata-learn 

Since you have [fix-embedlivesample-macros-learn](https://github.com/Lou8is/translated-content/tree/fix-embedlivesample-macros-learn), the commits for the EmbedLiveSample situation should be kept.
This way we have a "proper" history to review on this PR.

Please let me know if you need help in a "synchronous" way and I'll gladly help :)

@SphinxKnight
Copy link
Member

SphinxKnight commented Jul 19, 2023

since the changes now touch a lot of files (incl. other locales and some engine/core stuff who were added as reviewers), I'll go ahead for the push

@github-actions github-actions bot added merge conflicts 🚧 This pull request has merge conflicts that must be resolved. and removed merge conflicts 🚧 This pull request has merge conflicts that must be resolved. l10n-ja Issues related to Japanese 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. system Infrastructure and configuration for the project labels Jul 19, 2023
@github-actions github-actions bot removed the merge conflicts 🚧 This pull request has merge conflicts that must be resolved. label Jul 19, 2023
@Lou8is
Copy link
Contributor Author

Lou8is commented Jul 19, 2023

Yeah thanks for handling that. I obviously messed up with git.
I'll handle the EmbedSampleLive in the ongoing PR :)

@SphinxKnight SphinxKnight merged commit 4577628 into mdn:main Jul 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
l10n-fr Issues related to French content.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants