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

Get rid of innerHTML #115

Closed
wagnerand opened this Issue Oct 19, 2016 · 10 comments

Comments

Projects
None yet
3 participants
@wagnerand

wagnerand commented Oct 19, 2016

Reviewers discourage the use of innerHTML for security and performance reasons. Let's live up to our own best practices. :)

There are currently only two occurrences:

history-deleter/history.js
17:  document.getElementById('history').innerHTML = `No history for ${hostname}.`;

navigation-stats/popup.js
17:  listEl.innerHTML = "";
@Hrily

This comment has been minimized.

Contributor

Hrily commented Oct 20, 2016

I would like to work upon it...
Is it okay to use DOM methods?

@wbamberg

This comment has been minimized.

Member

wbamberg commented Oct 20, 2016

I would like to work upon it...

Great!

Is it okay to use DOM methods?

Yes!

@Hrily

This comment has been minimized.

Contributor

Hrily commented Oct 20, 2016

@wbamberg
Can you tell me what does the ${} operator do?
I'm not able to get it.

@wbamberg

This comment has been minimized.

Member

wbamberg commented Oct 20, 2016

It's for interpolation in template literals: "Template literals are string literals allowing embedded expressions."

-> https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Template_literals.

@Hrily

This comment has been minimized.

Contributor

Hrily commented Oct 20, 2016

So, `` no different than a "" string?

@Hrily Hrily referenced this issue Oct 20, 2016

Merged

Removed innerHTML #116

@wbamberg

This comment has been minimized.

Member

wbamberg commented Oct 20, 2016

No, it is different.

`` is used to create a template literal, which enables you to do string interpolation:

var sub = "World";
var greet1 = `Hello ${sub}` // -> "Hello World"
var greet2 = "Hello ${sub}" // -> "Hello ${sub}"
@Hrily

This comment has been minimized.

Contributor

Hrily commented Oct 20, 2016

Okay, I didn't know about them...

@Hrily

This comment has been minimized.

Contributor

Hrily commented Oct 20, 2016

I have made a PR. #116
Can you review it?

@wbamberg

This comment has been minimized.

Member

wbamberg commented Oct 22, 2016

Merged #116. Thanks @Hrily !

@wbamberg wbamberg closed this Oct 22, 2016

@Hrily

This comment has been minimized.

Contributor

Hrily commented Oct 22, 2016

Welcome :)
Even if it is a small modification, I'm feeling proud as my first Open Source contribution is to the MDN :D

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment