phoneApp #19

NataliaButenko · 2018-07-24T08:44:51Z

No description provided.

OlegLustenko · 2018-07-24T13:48:05Z

phoneApp/index.js

+
+	render() {
+
+		document.body.innerHTML += this.createHeader();


oyy!
as I see you updating innerHTML several times, it's better to update it only once

OlegLustenko · 2018-07-24T13:49:04Z

phoneApp/index.js

+			<footer class="footer">
+				<div class="container bottom-radius">
+					<nav class="main-nav">
+						<a href="index.html" class="tab active">


I believe you could make links more reusable.

What if one day you have to set class but the click ? or something more expensive.

I guess it could look that way

<nav class="main-nav"> this.renderLink({ content:'Contacts', className:'active', icon:'search'}); this.renderLink({ content:'Keypad', icon:'th'}); this.renderLink({ content:'Edit contact' }); ... </nav>

We could reuse some HTML parts and create a smaller reusable blocks

OlegLustenko · 2018-07-24T13:49:36Z

phoneApp/index.js

+		document.body.innerHTML += this.createHeader();
+		document.body.innerHTML += this.createMain();
+		let insert = document.querySelector('main > div');
+		insert.appendChild(this.createTable());


pretty sure you have to create table with inner HTML also

OlegLustenko · 2018-07-24T13:50:30Z

phoneApp/index.js

+
+	render() {
+
+		document.body.innerHTML += this.createHeader();


document.body.innerHTML not sure I like to use document.body.

You have to create an additional tag at index.html and call it, for example,

<div id="mountNode"></div>

And update content only inside such node. updating the whole document.body is too risky

…v1/javascript-react-course-NataliaButenko into phoneApp

OlegLustenko

I see a lot of job already done, but it requires updates in architecture.

And I don't see your main class called `App' who initializing all pages and store all App specific state.

For the first experience with HTML and not first with JavaScript :), it's a pretty good result. Let's improve it together!

OlegLustenko · 2018-08-15T12:23:16Z

phoneApp/keypad.js

+    		if(e.target.tagName === 'BUTTON') {
+    			let input =  document.querySelector('input.numbers');
+    			if(input.value.length === 0) {
+    				input.value += `(${e.target.textContent}`;


it's better to use regexp here.
Such logic with length is pretty hard to read and extend

OlegLustenko · 2018-08-15T12:24:21Z

phoneApp/keypad.js

+  		input.onkeypress = function(event) {
+  			if((event.charCode >= 48 && event.charCode <=57) || event.charCode == 42 || event.charCode == 35) {
+    			if(input.value.length === 0) {
+    				input.value += `(${event.target.textContent}`;


well, it looks like a duplication of the code from line 43.

You should create reusable function to generate such numbers (and use regexp)

OlegLustenko · 2018-08-15T12:26:17Z

phoneApp/keypad.js

+  			input.value = input.value.slice(0, -1);
+  		});
+  		let input = document.querySelector('input.numbers');
+  		input.onkeypress = function(event) {


you have to use addEventListener for keypress intead of onkeypress event binding

OlegLustenko · 2018-08-15T12:27:16Z

phoneApp/keypad.js

+
+	addEvents() {
+		let keypadHolder = document.querySelector('div.keypad-holder');
+  		keypadHolder.addEventListener('click', function(e) {


in most scenarios of adding callback function to event you have to create additional method and than bind it, the same as we done it in classroom on MVC lesson

OlegLustenko · 2018-08-15T12:30:37Z

phoneApp/keypad.js

@@ -0,0 +1,91 @@
+class Keypad {
+	constructor() {
+		this.buttonsContent = ['1', '2', '3', '4', '5', '6', '7', '8', '9', '*', '0', '#'];


We should create numbers as static content, no need to store them in any structures.

How to understand what we should store in a variable and what is just static?

If such data sent from the server you have to store it scenarios (around 95% of all scenarios) it's just a static data.

Furthermore, there will never exist numbers like "11" on keypad at phone :)

OlegLustenko · 2018-08-15T12:54:16Z

phoneApp/userContacts.js

+		// 	email: '[email protected]' 
+		// }	
+		//];
+		this.columnHeadings = ['Name', 'Last name', 'Email'];


please move such columns to static innerHTML

OlegLustenko · 2018-08-15T12:54:58Z

phoneApp/userContacts.js

+				start += `<th>${elem}</th>\n`
+				return start;
+			}, '');
+			let resultThead = openThead + `${createThead}</tr></thead>`;


puppies dies in the loop :(

I mean please make 1 ES6 string as with example I mentioned above and make it single line

OlegLustenko · 2018-08-15T12:55:12Z

phoneApp/userContacts.js

+			`;
+			let createTbody = dataUsers.reduce((start, elem) => {
+				start += `
+					<tr data-uid="${elem.id}">


the same about string concatenation

OlegLustenko · 2018-08-15T12:55:24Z

phoneApp/userContacts.js

+
+		table.addEventListener('click', function(e) {
+			if(e.target.tagName === 'TH') {
+        sortTable(e.target.cellIndex, e.target.textContent);


please fix formatting

OlegLustenko · 2018-08-15T12:56:49Z

phoneApp/userContacts.js

+      	let newUsers = userJson.map((elem) => {
+      		return {
+      			email: elem.email,
+            name: elem.fullName.split(' ')[0],


please optimize such call elem.fullName.split(' ')

I have provided an a example several days ago in skype.
Feel free to send message me with any questions

phoneApp

15f323a

OlegLustenko requested changes Jul 24, 2018

View reviewed changes

NataliaButenko and others added 7 commits July 31, 2018 16:23

Merge branch 'master' of https://github.com/EasyCode-JavaScriptReact-…

14228cb

…v1/javascript-react-course-NataliaButenko into phoneApp

update phoneApp

4ff03c0

update phoneApp

eee1fa2

update phoneApp (07.08.18)

283863e

update phoneApp (10.08.18)

7725d89

update phomeApp (12.08.18)

3b946f2

update phoneApp (13.08.18)

59872b9

NataliaButenko force-pushed the phoneApp branch 3 times, most recently from 026cde8 to 59872b9 Compare August 13, 2018 20:18

update phoneApp (14.08.18)

ec0a97f

OlegLustenko requested changes Aug 15, 2018

View reviewed changes

NataliaButenko added 2 commits August 21, 2018 14:49

update phoneApp (21.08.18)

cbf117d

update phoneApp

8baa4ff

phoneApp #19

Are you sure you want to change the base?

phoneApp #19

Uh oh!

Conversation

NataliaButenko commented Jul 24, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

OlegLustenko left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants