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

Stop transcluding live sample sources #15872

Closed
36 tasks done
wbamberg opened this issue May 10, 2022 · 16 comments · Fixed by #16118
Closed
36 tasks done

Stop transcluding live sample sources #15872

wbamberg opened this issue May 10, 2022 · 16 comments · Fixed by #16118
Labels
MDN:Project Anything related to larger core projects on MDN

Comments

@wbamberg
Copy link
Collaborator

wbamberg commented May 10, 2022

The {{EmbedLiveSample}} macro has a feature in which you can ask to include sources not from the current page but from a different page.

According to @escattone this along with {{page}} was one of the things that made KS much more complicated than it needed to be (because it introduced a build dependency between source and target).

Now we are close to 0 occurrences of {{page}} in mdn/content (thanks mostly to @hamishwillee ) it would be nice to remove this other source of complexity.

These are all the pages that make use of this feature of {{EmbedLiveSample}}:

@github-actions github-actions bot added the needs triage Triage needed by staff and/or partners. Automatically applied when an issue is opened. label May 10, 2022
@teoli2003
Copy link
Contributor

Command used to generate that list: rg "EmbedLiveSample *\(.*,.*,.*, *'.?'.*\)" -l | wc -l

@sideshowbarker sideshowbarker removed the needs triage Triage needed by staff and/or partners. Automatically applied when an issue is opened. label May 11, 2022
@wbamberg
Copy link
Collaborator Author

Command used to generate that list: rg "EmbedLiveSample *\(.*,.*,.*, *'.?'.*\)" -l | wc -l

This is too loose, as it is catching lines like https://github.com/mdn/content/blob/main/files/en-us/web/css/filter/index.md?plain=1#L177:

{{EmbedLiveSample('blur','100%','236px','','', 'no-codepen')}}

@teoli2003
Copy link
Contributor

teoli2003 commented May 12, 2022

I think this is better: rg "EmbedLiveSample *\([^,]*,[^,]*,[^,]*,[^,]*, *'[^'][^']*'" -i

(I've updated the link in the OP)

@hamishwillee
Copy link
Collaborator

Excellent idea. Once we're done, I think we should explicitly prohibit this macro importing from other pages.

@teoli2003
Copy link
Contributor

So, most of them were fake transclusions 😄

@teoli2003
Copy link
Contributor

teoli2003 commented May 14, 2022

Excellent idea. Once we're done, I think we should explicitly prohibit this macro importing from other pages.

Given that this macro param is (still) used on mdn/translated-content, we can't just remove the mechanism associated with the parameter.

But maybe we can add a non-failing (à la mdn.deprecated()) flaw that would warn us if such a thing is used on mdn/content.

@OnkarRuikar
Copy link
Contributor

But maybe we can add a non-failing (à la mdn.deprecated()) flaw that would warn us if such a thing is used on mdn/content.

I think if we remove the usage from the documentation then we won't see new cases.
Also, contributors will prefer to write a new code instead of looking for existing code that achieves intended output. 🤔

@teoli2003
Copy link
Contributor

They can forget a parameter and suddenly there is something in the parameter that will break the system. I've seen numerous typos appearing in the flaw dashboard (and mixing the number of empty parameters is easy), despite the review system. I would prefer to have a security net here given I don't see transclusion going away soon in mdn/translated-content.

@OnkarRuikar
Copy link
Contributor

Need to update the list in OP. It shows 6 tasks remaining.

@OnkarRuikar
Copy link
Contributor

Looks like transclusion hasn't reached the conclusion 🙃:

The regexes didn't use double quotes.

@teoli2003
Copy link
Contributor

Let's reopen this.

@teoli2003 teoli2003 reopened this May 17, 2022
@github-actions github-actions bot added the needs triage Triage needed by staff and/or partners. Automatically applied when an issue is opened. label May 17, 2022
@sideshowbarker sideshowbarker removed the needs triage Triage needed by staff and/or partners. Automatically applied when an issue is opened. label May 18, 2022
@wbamberg
Copy link
Collaborator Author

GitHub needs a :sob reaction.

@Josh-Cena
Copy link
Member

For reference, here's a working regex you could use: EmbedLiveSample *\((?:[^,]*?,){4} *(['"])[^'"]+\1

@Josh-Cena Josh-Cena added the MDN:Project Anything related to larger core projects on MDN label Jun 23, 2023
@Josh-Cena
Copy link
Member

#28376 and #27813

@wbamberg
Copy link
Collaborator Author

wbamberg commented Aug 3, 2023

🎉 great work @queengooborg and @cw118 !

@hamishwillee
Copy link
Collaborator

What he said ^^^^ !!!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
MDN:Project Anything related to larger core projects on MDN
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants