# PR Review of ITDEV-117421
1. This is not a component, this is a webpage. Which means that we are not using React to its capability. Also passing HTML in like you did opens the site for XSS Attacks because elements are not escaped. We use tools like React for many reasons, but one of the larger reasons is that it abstracts away some of the _grunt_ work of creating an application. I think that your understanding of React is confusing your work. Through the help of Babel we taking a shortcut to render things to the DOM.
These 2 examples are the same:
```js
// what you need to write
const element = (
<h1 className="greeting">
Hello, world!
</h1>
);
// what babel does for you
const element = React.createElement(
'h1',
{className: 'greeting'},
'Hello, world!'
);
```
References:
- https://reactjs.org/docs/introducing-jsx.html
- https://reactjs.org/docs/thinking-in-react.html
2. Always use `const` or `let` to declare variables. Not doing so will result in global variables. We want to avoid polluting the global namespace. You should only use `var` for a *very specific* reason that can be justified by not just using `let`
```js
// bad
var superPower = new SuperPower()
// good
const superPower = new SuperPower()
```
3. Variable names should be `camelCase` and *not* `snake_case`.
```js
// bad
let variable_name = 'my name'
// good
let otherVariableName = '👍🏾'
```
4. Generally speaking, just avoid iterators. JavaScript has higher-order functions like `for-in` and `for-of` This enforces our immutable rule. Dealing with pure functions that return values is easier to reason about than side effects. Use `map()` / `every()` / `filter()` / `find()` / `findIndex()` / `reduce()` / `some()` / ... to iterate over arrays, and `Object.keys()` / `Object.values()` / `Object.entries()` to produce arrays so you can iterate over objects.
```js
const numbers = [1, 2, 3, 4, 5];
// bad
let sum = 0;
for (let num of numbers) {
sum += num;
}
sum === 15;
// good
let sum = 0;
numbers.forEach((num) => {
sum += num;
});
sum === 15;
// best (use the functional force)
const sum = numbers.reduce((total, num) => total + num, 0);
sum === 15;
// bad
const increasedByOne = [];
for (let i = 0; i < numbers.length; i++) {
increasedByOne.push(numbers[i] + 1);
}
// good
const increasedByOne = [];
numbers.forEach((num) => {
increasedByOne.push(num + 1);
});
// best (keeping it functional)
const increasedByOne = numbers.map(num => num + 1);
```
5. Stop using `innerHTML`. React uses a virtual DOM, when it goes to compare the diff against the actual DOM, it can straight up bypass checking the children of that node because it knows the HTML is coming from another source. So there's performance gains. More importantly, if you simply use `innerHTML`, React has no way to know the DOM node has been modified. The next time the render function is called, React will overwrite the content that was manually injected with what it thinks the correct state of that DOM node should be.
6. Stop using in line styles! Inline CSS will always, always win in precedence over any linked-stylesheet CSS. This can cause enormous headache for you if and when you go and write a proper cascading stylesheet, and your properties aren't applying correctly. It also hurts your application semantically: CSS is about separating presentation from markup. When you tangle the two together, things get much more difficult to understand and maintain. It's a similar principle as separating database code from your controller code on the server side of things.
7. Get rid of all of regEx stuff and use `.map() and .filter()` every single instance I have found where you are parsing some string and matching can be used by JS methods that are easier to maintain, write, and understand. Prefer them over convoluted RegEx.
- Reference:
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array
8. When you are accessing properties on an array, use dot notation
```js
const person = {
name: 'Dominique Hallan',
age: 36,
}
// bad
const personData = person['name]'
// good
const getPersonName = person.name
```
9. Do not use trailing or leading underscores. JavaScript does not have the concept of privacy in terms of properties or methods. Although a leading underscore is a common convention to mean “private”, in fact, these properties are fully public, and as such, are part of your public API contract. This convention might lead developers to wrongly think that a change won’t count as breaking, or that tests aren’t needed. tl;dr: if you want something to be “private”, it must not be observably present.
```js
// bad
this.__firstName__ = 'Panda';
this.firstName_ = 'Panda';
this._firstName = 'Panda';
// good
this.firstName = 'Panda';
// good, in environments where WeakMaps are available
// see https://kangax.github.io/compat-table/es6/#test-WeakMap
const firstNames = new WeakMap();
firstNames.set(this, 'Panda');
```
10. Stop saving references to `this`, instead use an arrow function or function bind
```js
// bad
function foo() {
const self = this;
return function () {
console.log(self);
};
}
// bad
function foo() {
const that = this;
return function () {
console.log(that);
};
}
// good
function foo() {
return () => {
console.log(this);
};
}
```
11. Stop using all those `div`s! I am going to sum up words right from HTML5 spec: "_Authors are strongly encouraged to view the div element as an element of last resort, for when no other element is suitable. Use of more appropriate elements instead of the div element leads to better accessibility for readers and easier maintainability for authors._"
Reference:
- https://www.w3.org/TR/html5/grouping-content.html#the-div-element
12. Components should begin with a capital letter.
13. Bind event handlers for the render method in the constructor
```js
// bad
class extends React.Component {
onClickDiv() {
// do stuff
}
render() {
return <div onClick={this.onClickDiv.bind(this)} />;
}
}
// very bad
class extends React.Component {
onClickDiv = () => {
// do stuff
}
render() {
return <div onClick={this.onClickDiv} />
}
}
// good
class extends React.Component {
constructor(props) {
super(props);
this.onClickDiv = this.onClickDiv.bind(this);
}
onClickDiv() {
// do stuff
}
render() {
return <div onClick={this.onClickDiv} />;
}
}
```
14. Do not use underscore prefix for internal methods of a React component. Underscore prefixes are sometimes used as a convention in other languages to denote privacy. But, unlike those languages, there is no native support for privacy in JavaScript, everything is public. Regardless of your intentions, adding underscore prefixes to your properties does not actually make them private, and any property (underscore-prefixed or not) should be treated as being public.
```js
// bad
React.createClass({
_onClickSubmit() {
// do stuff
},
// other stuff
});
// good
class extends React.Component {
onClickSubmit() {
// do stuff
}
// other stuff
}
```
I know that was a *bunch* of information, and I apologize. The point that I am trying to impress on you is that while your effort is very much apperciated, further study is going to be needed to grasp the concepts of React and larger than that, JavaScript. Without a deeper understanding of JavaScript at it core I can tell you from experience that you are just going to struggle. It is going to be my suggestion that this is redone and that you start spending a couple of hours at work to bring you up to speed on modern JavaScript.