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

Add DocumentFragment.getElementById() #24449

Merged
merged 17 commits into from
Mar 29, 2023
Merged

Conversation

teoli2003
Copy link
Contributor

This is part of openwebdocs/project#152

Added the missing method, and slightly improved its counterpart on Document.

@teoli2003 teoli2003 requested a review from a team as a code owner February 15, 2023 08:27
@teoli2003 teoli2003 requested review from wbamberg and removed request for a team February 15, 2023 08:27
@github-actions github-actions bot added the Content:WebAPI Web API docs label Feb 15, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Feb 15, 2023

Preview URLs

(comment last updated: 2023-03-29 17:36:20)

@wbamberg
Copy link
Collaborator

Taking a look.

Copy link
Contributor

@dawei-wang dawei-wang left a comment

Choose a reason for hiding this comment

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

A couple of grammatical edits

files/en-us/web/api/document/getelementbyid/index.md Outdated Show resolved Hide resolved
teoli2003 and others added 3 commits February 16, 2023 07:08
Co-authored-by: dawei-wang <dawei-wang@users.noreply.github.com>
Co-authored-by: dawei-wang <dawei-wang@users.noreply.github.com>
files/en-us/web/api/document/getelementbyid/index.md Outdated Show resolved Hide resolved
files/en-us/web/api/document/getelementbyid/index.md Outdated Show resolved Hide resolved
files/en-us/web/api/document/getelementbyid/index.md Outdated Show resolved Hide resolved

## Examples

### HTML
Copy link
Collaborator

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.

Copy link
Collaborator

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:

  • 3 buttons:
    • one labeled something like "Find elements" that looks for find Apple and Durian in the document fragment and document, and logs the results
    • one labeled "Add fragment to document" that adds the fragment
    • one labeled "Reset" that reloads the iframe, so the reader can try again

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:

output.append(document.createElement("br"));
output.append(
  document.createTextNode(
    `Found Apple in the fragment? ${
      fragment.getElementById("Apple") ? "Yes" : "No"
    }`
  )

...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:

output.textContent +=  `Found Apple in the fragment? ${
      fragment.getElementById("Apple") ? "Yes" : "No"
    }\n`

(e.g. https://developer.mozilla.org/en-US/docs/Learn/JavaScript/Building_blocks/Events#bubbling_example)

@teoli2003
Copy link
Contributor Author

Note to self here: I need to improve the example per Will's comment.

@teoli2003
Copy link
Contributor Author

@wbamberg Is the example better this way?

@teoli2003 teoli2003 requested a review from wbamberg March 27, 2023 08:56
@wbamberg
Copy link
Collaborator

I'm not sure what this example is supposed to say. In the old example, I think, we did:

  1. Create Durian in the doc, and a fragment containing Apple: now Durian is in the doc, and Apple is in a fragment
  2. Append the fragment to the doc: now Apple and Durian are both in the doc, and neither are in the fragment

So I think this is showing me:

  • that I can use documentFragment.getElementById() to find elements in the fragment
  • that once I've added the fragment to the document, the elements are no longer in the fragment(?)

But in this version, we don't call documentFragment.getElementById() at all, we:

  1. Create Cherry in the doc
  2. On "Add fragment to document", create a fragment and add it to the doc

So after (1), we only have Cherry, after it we have Cherry and Apple. This doesn't show me anything about documentFragment.getElementById().

Comment on lines 41 to 59
```html
<button id="reset">Reset example</button>
Basket content:
<ul>
<li id="Cherry">Cherry</li>
</ul>
<button id="add">Add fragment to document</button>

Console: <button id="find">Find elements</button>
<pre id="log" />
```

```css hidden
#reset,
#add {
display: block;
margin-bottom: 10px;
}
```
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
```html
<button id="reset">Reset example</button>
Basket content:
<ul>
<li id="Cherry">Cherry</li>
</ul>
<button id="add">Add fragment to document</button>
Console: <button id="find">Find elements</button>
<pre id="log" />
```
```css hidden
#reset,
#add {
display: block;
margin-bottom: 10px;
}
```
```html
<button id="find">Find elements</button>
<button id="add">Add fragment to document</button>
<button id="reset">Reset example</button>
<div id="basket">
Basket content:
<ul>
<li id="Cherry">Cherry</li>
</ul>
</div>
Console:
<pre id="log"></pre>
```
```css hidden
#basket {
margin: 10px 0;
}

(just a suggestion, but I think it would look much nicer to have the buttons in a row at the top, in the order in which we expect people might click them)


### Extend a list of elements

This example creates a list with one element, `Cherry`, while a `Find elements` button will look for `Apple` and `Cherry` and logs it. The button `Add fragment to document` will insert four extra elements in the list, changing the results of pressing the `Find elements` button. Finally, a `Reset` button sets the example in its original state.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Like I say, I'm not sure what this example is intended to show.

But I think it is worth spelling things out:

  • what the example contains
  • what users can do
  • what they will see
  • why they will see it

For example, from the previous version of this we could say something like:

The document contains a list of fruit. Initially this list contains a single item "Durian". We create a DocumentFragment object containing four more fruits, "Apple", "Orange", "Banana", and "Melon".

If you click "Find elements", we use getElementById() to look for "Durian" and "Apple" in both the document and the fragment. We find "Durian" in the document and "Apple" in the fragment.

If you click "Add fragment to document", we add the fragment to the list in the document.

Now, if you click "Find elements" again, we find both "Durian" and "Apple" in the document, and neither of them in the fragment. This is because appending a fragment to a document moves the fragment's nodes into the DOM, leaving behind an empty DocumentFragment.

Comment on lines 102 to 121
const resetButton = document.getElementById("reset");
resetButton.addEventListener("click", () => {
// Empty the list
while (ul.firstChild) {
// The list is LIVE so it will re-index each call
ul.removeChild(ul.firstChild);
}

// Create 'Cherry` entry and adds it to the list
const li = document.createElement("li");
li.textContent = "Cherry";
li.id = "Cherry";
ul.append(li);

// Empty log
log.textContent = "";

// (Re-)Activate the 'add' button
addButton.disabled = false;
});
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
const resetButton = document.getElementById("reset");
resetButton.addEventListener("click", () => {
// Empty the list
while (ul.firstChild) {
// The list is LIVE so it will re-index each call
ul.removeChild(ul.firstChild);
}
// Create 'Cherry` entry and adds it to the list
const li = document.createElement("li");
li.textContent = "Cherry";
li.id = "Cherry";
ul.append(li);
// Empty log
log.textContent = "";
// (Re-)Activate the 'add' button
addButton.disabled = false;
});
const resetButton = document.getElementById("reset");
document.location.reload();
});

This is what I usually do here.

@teoli2003
Copy link
Contributor Author

Yes, I forgot about the fragment (!).

So I rewrote the example:

  • The buttons are on a single line at the top in the order of expected click
  • There is a pseudo-console at the bottom listing the results of the different getElementById() calls. There is no button to log, it is called after a click on the buttons. (It is always up-to-date)
  • I have two lists: one is the ul and the other is the fragment viewer.

The code is shorter (but logging is still a significant part of it)

@teoli2003 teoli2003 requested a review from wbamberg March 29, 2023 07:40
Copy link
Collaborator

@wbamberg wbamberg left a comment

Choose a reason for hiding this comment

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

This looks great to me, thank you! I had a suggestion to expand the description a bit.

Co-authored-by: wbamberg <will@bootbonnet.ca>
@teoli2003 teoli2003 merged commit 64c6d26 into mdn:main Mar 29, 2023
@teoli2003
Copy link
Contributor Author

Thank you for the thorough reviews!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Content:WebAPI Web API docs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants