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

fix(playground): preserve HTML comments #11304

Merged
merged 1 commit into from
Jul 4, 2024

Conversation

jeremyredhead
Copy link
Contributor

This fixes MDN examples involving comments (e.g. https://developer.mozilla.org/en-US/docs/Web/API/CharacterData/data), as well as being the Correct thing to do.

I also took the liberty of converting that == into an === since I am 99.999% sure the == was not intended.

Summary

It fixes this example of reading a Comment's data. There may be other examples that are fixed by this, IDK, I don't have it in me to even try to figure out how to figure that out.

I did not file an issue first because I am tired, and I do not understand the point of filing an issue first if I already have the code to fix the problem, and I am fairly certain the issue is not contentious (or at least, I don't think it should be...?)

I also did not run yarn, or sign my commits, sorry. That's a lot of work to put in for a small drive-by fix. If this is a blocker but the actual code looks OK, feel free to just steal it & re-apply in a proper commit. I'd like to be credited if so but I'd rather the fix get committed without any credit to me than the fix not get committed at all.

Problem

Comments are discarded (elided?) when running the playground.

Solution

It clones comments too instead of discarding them.


How did you test this change?

I did not; the code is extremely simple & very obviously correct.

This fixes MDN examples involving comments (e.g. https://developer.mozilla.org/en-US/docs/Web/API/CharacterData/data),
as well as being the Correct thing to do.
@jeremyredhead jeremyredhead requested a review from a team as a code owner June 12, 2024 15:13
@caugner caugner changed the title Preserve HTML comments in playground fix(playground): preserve HTML comments Jun 14, 2024
@caugner
Copy link
Contributor

caugner commented Jun 14, 2024

This will be deployed to our stage system with the next nightly build, and we'll take a look next week. Thanks!

@caugner
Copy link
Contributor

caugner commented Jun 16, 2024

This will be deployed to our stage system with the next nightly build, and we'll take a look next week. Thanks!

@jeremyredhead The result changed somewhat on stage, but it doesn't seem to be correct yet: https://developer.allizom.org/en-US/docs/Web/API/CharacterData/data#reading_a_comment_using_data

@caugner caugner marked this pull request as draft June 18, 2024 10:53
@jeremyredhead
Copy link
Contributor Author

The result changed somewhat on stage, but it doesn't seem to be correct yet: https://developer.allizom.org/en-US/docs/Web/API/CharacterData/data#reading_a_comment_using_data

Oops! I was so focused on fixing the playground that I didn't even notice that the example code itself was questionable and apparently also needs fixing. (i am guessing the previous version(s) of the live example sandbox had some element or whitespace that always preceded the contents? i am unsure)

Elaboration on ways content could be fixed

The simplest way to fix it would be to change the first line in the example to

const comment = document.body.childNodes[0];

but I wonder if that is too fragile and prone to breaking again in the future.

Another way would be to filter for comment nodes, but this makes for a much longer line and is maybe too advanced for noobs beginners?

const comment = [...document.body.childNodes].filter(node => node.nodeName === '#comment')[0]

The most reliable(?) method but still relatively simple method I can think of is is wrapping the comment in an element with an ID, like

<div id="comment"><!-- This is an HTML comment --></div>

and then changing the first line of the JS to

const comment = document.getElementById("comment").childNodes[0];

but I am concerned this would set a bad precedent/example for new learners regarding comments...

Either way, in the end, the patch I submitted is correct and works, it's just the content needs to be changed in concert with it (unless, assuming my aforementioned hypothesis is true, attempts are made to emulate the implementation details of previous versions of the live examples, which seems a bad idea)

@caugner caugner marked this pull request as ready for review July 4, 2024 16:38
@caugner caugner merged commit e30b1a9 into mdn:main Jul 4, 2024
8 checks passed
@caugner
Copy link
Contributor

caugner commented Jul 4, 2024

Oops! I was so focused on fixing the playground that I didn't even notice that the example code itself was questionable and apparently also needs fixing

@jeremyredhead You're right, it looks like the example assumes that the comment is the second element in the body. A solution might be to wrap the comment and the other element in a <section id="body"> and get the comment via document.getElementById("body").childNodes[0]. Can you open a PR in mdn/content? 🙏

ferdnyc pushed a commit to ferdnyc/yari that referenced this pull request Jul 13, 2024
Problem: Comments were discarded when running an example in the Playground.

Solution: Clone comments as well, instead of discarding them.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants