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

Jump to bottom

misc: replace appendChild with append #13995

Merged
merged 3 commits into from
May 16, 2022
Merged

misc: replace appendChild with append #13995

merged 3 commits into from
May 16, 2022

Conversation

alexnj
Copy link
Member

@alexnj alexnj commented May 11, 2022 •
edited
Loading

Fixes #13991

Replaces all .appendChild() occurrences with .append().

@alexnj
misc: replace most .appendChild() with .append().
022052d 
Except instances where the return value is needed/used.
@alexnj alexnj requested a review from a team as a code owner May 11, 2022 21:41
@alexnj alexnj requested review from connorjclark and removed request for a team May 11, 2022 21:41
@adamraine
Copy link
Member

Is it worth it to add a lint rule. I'm definitely going to forget this and use appendChild at some point.

@connorjclark connorjclark changed the title misc: replace most of .appendChild() usage with .append(). report: replace most of .appendChild() with .append() May 11, 2022
@connorjclark connorjclark changed the title report: replace most of .appendChild() with .append() report: replace most of .appendChild with .append May 11, 2022
@connorjclark connorjclark changed the title report: replace most of .appendChild with .append misc: replace most of .appendChild with .append May 11, 2022
paulirish
paulirish reviewed May 11, 2022
View reviewed changes
report/renderer/category-renderer.js Outdated Show resolved Hide resolved
report/renderer/crc-details-renderer.js Outdated Show resolved Hide resolved
report/renderer/crc-details-renderer.js Outdated Show resolved Hide resolved
@paulirish
Copy link
Member

nice PR :)

@paulirish
Copy link
Member

paulirish commented May 11, 2022 •
edited
Loading

Is it worth it to add a lint rule. I'm definitely going to forget this and use appendChild at some point.

😱 you wouldn't DARE

edit: but no i dont think its worth it to do a lint rule. any appendChild will def get caught in review going forward. me and connor certainly have a vendetta against it. :)

@brendankenny
Copy link
Member

Is it worth it to add a lint rule. I'm definitely going to forget this and use appendChild at some point.

I feel like no. It doesn't really matter a lot of the time. The biggest win is el.append(...lotsOChildren), and we can't just outright ban it because sometimes you do want the return value.

@alexnj alexnj changed the title misc: replace most of .appendChild with .append misc: replace all of .appendChild with .append May 12, 2022
@alexnj
Copy link
Member Author

alexnj commented May 12, 2022

Adjusted for review comments and we should be appendChild-free by now. :)

@alexnj alexnj changed the title misc: replace all of .appendChild with .append misc: replace all occurrences of .appendChild with .append May 12, 2022
@alexnj alexnj requested a review from paulirish May 12, 2022 13:40
connorjclark
connorjclark approved these changes May 13, 2022
View reviewed changes
Copy link
Collaborator

@connorjclark connorjclark left a comment

Choose a reason for hiding this comment

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

LGTM, left a couple nits

lighthouse-cli/test/fixtures/byte-efficiency/tester.html Outdated Show resolved Hide resolved
lighthouse-core/test/lib/page-functions-test.js Outdated Show resolved Hide resolved
@connorjclark connorjclark changed the title misc: replace all occurrences of .appendChild with .append misc: replace appendChild with append May 13, 2022
@connorjclark connorjclark merged commit 4e5d5d1 into GoogleChrome:master May 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@connorjclark connorjclark connorjclark approved these changes

@paulirish paulirish Awaiting requested review from paulirish

Assignees

@connorjclark connorjclark

Labels
waiting4reviewer
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Replace .appendChild with .append
6 participants
@alexnj @adamraine @paulirish @brendankenny @connorjclark @devtools-bot