Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add DocumentFragment.getElementById() #24449
Add DocumentFragment.getElementById() #24449
Changes from 2 commits
b16cbf9
5fd501b
51fb966
0613d3d
7e9862d
670e982
6893723
02dc80a
cb67409
2f9f278
2ca2505
4e563f1
af534b7
427c408
ac3c4ac
772f0f6
733047b
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's good practice to have a descriptive H3 for the whole example, and to make the "HTML" etc headings H4 headings under that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are kind of picky comments, because this example does work fine and illustrate what it wants to. So please feel free to ignore, or use whatever parts you want to.
The behavior of this example isn't very easy to understand, and there is a lot of code to read - almost 100 lines.
I think with examples like this where we are basically showing state changes, it's better to make them more interactive, so the reader can see the current state and move on to the next state. Otherwise by the time the page is loaded, the whole story is told and I have to piece together what happened.
So maybe consider:
That way it's easier to understand the initial state and how the state changes when we add the fragment.
It's also I think preferable to include explanatory text in prose, outside the example, rather than inside (for example, it means people who have dark theme for readability will get the benefit of that.
Finally, the code that logs the results is quite verbose:
...and this contributes to this example looking long. This is really just framework code (i.e. it's not an essential part of the example) so it is distracting. In similar cases I make the output a
<pre>
and just do:(e.g. https://developer.mozilla.org/en-US/docs/Learn/JavaScript/Building_blocks/Events#bubbling_example)