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.

Sign up for GitHub

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

Change .append() to .appendChild() #953

Merged
merged 2 commits into from
Jun 27, 2022
Merged

Change .append() to .appendChild() #953

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f5cb301
Update plot.js to use .appendChild() rather than .append()
jkenzer Jun 25, 2022
e09c9c8
Refactor to use ternary and typeof check
jkenzer Jun 25, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions src/plot.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -222,11 +222,12 @@ export function plot(options = {}) {
if (caption != null || legends.length > 0) {
figure = document.createElement("figure");
figure.style.maxWidth = "initial";
figure.append(...legends, svg);
if (legends.length > 0) figure.appendChild(...legends);
Copy link
Member

Choose a reason for hiding this comment

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

appendChild unlike append only takes a single argument, so this will need to be a for loop now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it. Easy change. I think I was assuming that there would only ever be one legend in the array. I'll make this a for of loop.

figure.appendChild(svg);
if (caption != null) {
const figcaption = document.createElement("figcaption");
figcaption.append(caption);
figure.append(figcaption);
typeof caption === "object" ? figcaption.appendChild(caption) : (figcaption.innerHTML = caption);
Copy link
Member

Choose a reason for hiding this comment

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

We’ll need a more precise test for an element or text node here. I think we can crib from hypertext literal for the exact logic. And then in the non-element case, we’ll need to create a text node. (We shouldn’t set innerHTML since that would allow the caption to contain HTML and potentially cause encoding issues.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Gotcha about the innerHTML. I'm looking at the code now from the hypertext literal. I might need assistance here but let me see what I come up with.

Thanks for the feedback.

figure.appendChild(figcaption);
}
}

Expand Down