Phone app by viskosa · Pull Request #17 · EasyCode-JavaScriptReact-v1/javascript-react-course-viskosa

viskosa · 2018-07-20T14:11:30Z

No description provided.

viskosa · 2018-07-21T19:44:03Z

I've done virtualizing of contacts page. There are two solutions, the second I like more

OlegLustenko · 2018-07-20T18:29:29Z

js-core/phoneApp/main.js

+];
+
+const captions = ['Name', 'Last name', 'Email'];
+const footerContent = [


footer content is a static no need to virtualize it

OlegLustenko · 2018-07-20T18:30:10Z

js-core/phoneApp/main.js

+  }
+]
+
+const contactsPage = {


please use class instead of object

OlegLustenko · 2018-07-22T04:25:17Z

phoneApp/contacts.js

+        email: 'ViktorKriv@ec.ua'
+      }
+    ];
+    this.footerContent = [


it's just a static content, not a data that should be virtualized.

The difference between static content in "virtualized" for example,
The server would never know how many pages do you have but the server would know about users.

So you make a decision based on that knowledge

viskosa · 2018-07-23T09:54:53Z

Footer was made like a static content

OlegLustenko · 2018-07-31T09:23:01Z

phoneApp/common/footer.js

+  }
+
+  renderFooter() {
+    document.body.innerHTML += this.createFooter();


please do not use document.body :)

create an additional tag and work inside it.
You can meet some unpredictable behavior if you rewrite the whole body content. It will be very hard to debug it and found a mistake

OlegLustenko · 2018-07-31T09:23:22Z

phoneApp/common/footer.js

+  }
+
+  createFooter(){
+      return `<footer class="footer">


you can just place such html-layout inside render

OlegLustenko · 2018-07-31T09:24:14Z

phoneApp/keypad.js

+  buttonsHandler(){
+	let buttonsParent = document.querySelector('.keypad-holder');
+
+	buttonsParent.addEventListener('click', this.clickHandler)


are you sure it works correctly?

OlegLustenko · 2018-08-03T18:30:24Z

phoneApp/index.html

+					<tr>
+						<td>Влад</td>
+						<td>Яма</td>
+						<td>VladYama@ec.ua</td>


feel free to delete commented part

OlegLustenko · 2018-08-03T18:31:03Z

phoneApp/keypad.js

+class KeypadPage {
+  constructor(){
+  	this.title = 'Keypad';
+    this.buttonsValues = ['1', '2', '3', '4', '5', '6', '7', '8', '9', '*', '0', '#', ''];


please move all content specifiec part to innerHTML it's better to omit such data inside

OlegLustenko · 2018-08-03T18:36:10Z

phoneApp/keypad.js

+                    <span class = "tab-text">Contacts</span>
+                  </a>
+                  <a href="keypad.html" class="tab">
+                    <span class="glyphicon glyphicon-th" aria-hidden="true"></span>


this.renderLink({glyphicon:'pencil', href:'keypad.html'}) this.renderLink({glyphicon:'th', href:'contacts'}) this.renderLink({glyphicon:'pencil', href}) this.renderLink({glyphicon:'pencil', href})

OlegLustenko · 2018-08-03T18:37:59Z

phoneApp/keypad.js

+  }
+
+  render(){
+    document.body.innerHTML = this.renderHeader() + this.renderMain() + this.renderFooter();


please move all HTML specific content to render, and remove additional properties
for example:
renderHeader, renderMain, renderFooter

OlegLustenko · 2018-08-03T18:38:23Z

phoneApp/main.js

+  }
+]
+
+const contactsPage = {


please create class ContactsPage and move all logic there

OlegLustenko · 2018-08-03T18:39:25Z

phoneApp/main.js

+    let nav = this.newEl('div', null, {className: 'main-nav'});
+
+    footerContent.forEach((item) => {
+      let link = this.newEl('a', null, {className: `tab ${item.additionalClass}` , hrefAttr: item.href } );


please create innerHTML instead of createElement

OlegLustenko · 2018-08-03T18:39:41Z

phoneApp/main.js

+  },
+
+  setAttribute(element, attributes) {
+    let {className, typeAttr, forAttr, idAttr, placeholderAttr, hrefAttr, ariahiddenAttr} = attributes;


<3 super method/function

OlegLustenko · 2018-08-10T05:13:12Z

phoneApp/js/app.js

+
+        this.pages = {
+            contacts: new ContactsPage(this.state),	// тут передали ссылку на this.state
+			adduser: new AddUser(this.state),


I guess there a typo with formatting

OlegLustenko · 2018-08-10T05:14:08Z

phoneApp/js/app.js

+         </footer>`;
+
+         //this.initializeRouterHandlers();
+         this.appDOMNode = mountNode.querySelector('#app');	// сюда будем делать рендер всех страниц 


getElementById working around in 10 times faster than querySelector, I guess it's better to switch it

OlegLustenko · 2018-08-10T05:16:30Z

phoneApp/js/app.js

+    	parent.addEventListener('click', (e) => {
+    		e.preventDefault();
+    		//let target = e && e.target;
+    		let target = e && e.target && (e.target.closest('a') || e.target.classList.contains('tab'));


oh... such checks look like a bit overhead.
You can add additional attributes to every link and indicate "user clicked on link" that way

on real word usage probably we should be always 100% be sure about our target

OlegLustenko · 2018-08-10T05:19:01Z

phoneApp/js/app.js

+
+    		if (target.classList.contains('active')) return;
+    		if (target.classList.contains('tab')) {
+    			let active = document.querySelector('.active');


querySelector is always performance costly operation it better to omit every time you can do it.

So, for example, you could the same solution as we did with a slider in class.

OlegLustenko · 2018-08-10T05:23:48Z

phoneApp/js/app.js

+
+    			let href = target.getAttribute('href');
+    			//console.log(href);
+    			this.state.activePage = href.replace(/-/g, '').toLowerCase();


probably href.replace(/-/g, '').toLowerCase() such transformation adding new mental layout for understanding your app. You could just make "already" prepared data in the HTML without requires to transform it in future.

Or you can just use your HTML-attribute to indicate path of navigation and build "Map" to connect your active page

OlegLustenko · 2018-08-10T05:24:51Z

phoneApp/js/user.js

+			</div>`;
+  }
+
+  renderLink(options) {


I guess it's not used anymore? we may delete it

OlegLustenko · 2018-08-10T05:25:42Z

phoneApp/js/user.js

@@ -0,0 +1,70 @@
+class User {
+  	constructor(globalState){
+    this.state = globalState; //стал равен this.state-у со страницы App.js


something went wrong with the formatting. Try to install prettier to improve your code-base formatting.

OlegLustenko · 2018-08-10T05:29:26Z

phoneApp/js/app.js

+
+    }
+
+    initializeRouter () {


pretty sure you can create an additional file and call it "Router.js" - and move all specific to navigation logic in.

Then initialize as other pages.

The main goal what we trying to solve the minimal splitting by responsibilities have you feel it?

OlegLustenko · 2018-08-10T05:29:49Z

phoneApp/js/app.js

+	}
+
+	renderLink(options) {
+		let {href, glyphicon, text, active} = options;


it could be moved to Router.js also

OlegLustenko · 2018-08-10T05:30:44Z

phoneApp/js/app.js

+
+
+    switchRouter() {
+    	const parent = document.querySelector('.main-nav');


I think we can make such selector once - and remember the link to it, no need to make it on every

OlegLustenko · 2018-08-13T13:15:37Z

phoneApp/js/addUser.js

-  	let buttonsParent = document.querySelector('main');
-  	buttonsParent.addEventListener('click', this.clickHandler.bind(this));
-  }
+	buttonsHandler() {


so huge tabs

OlegLustenko · 2018-08-13T13:15:58Z

phoneApp/js/addUser.js

-  	buttonsParent.addEventListener('click', this.clickHandler.bind(this));
-  }
+	buttonsHandler() {
+		let buttonsParent = document.querySelector("main");


please use const instead of let

OlegLustenko · 2018-08-13T13:17:54Z

phoneApp/js/editContact.js

+			e &&
+			e.target &&
+			(e.target.closest("button") ||
+				e.target.classList.contains("add-btn"));


what if another dev would change that className at button - javascript would break ?
We shouldn't rely on classsNames

OlegLustenko · 2018-08-13T13:18:20Z

phoneApp/js/editContact.js

+			e.target &&
+			(e.target.closest("button") ||
+				e.target.classList.contains("add-btn"));
+		if (active == false) return;


if(!active) { return }

https://github.com/airbnb/javascript#comparison--shortcuts

OlegLustenko · 2018-08-13T13:20:12Z

phoneApp/js/editContact.js

+		if (active == false) return;
+
+		let input = active.querySelector("input");
+		input.style.backgroundColor = "lightgreen";


probably you can make it something like with CSS

:focus { }

JavaScript not really required there

OlegLustenko · 2018-08-13T13:20:49Z

phoneApp/js/keypad.js

-  	buttonsParent.addEventListener('click', this.clickHandler.bind(this, placeToInsertNumbers));
-    window.addEventListener('keypress', this.keyHandler.bind(this, placeToInsertNumbers));
+  buttonsHandler() {
+    let buttonsParent = document.querySelector("main");


please you const keyword instead of let

OlegLustenko · 2018-08-13T13:21:55Z

phoneApp/js/keypad.js

+      "click",
+      this.clickHandler.bind(this, placeToInsertNumbers)
+    );
+    window.addEventListener(


are ever remove such listener?
for example when-even you changing a page

OlegLustenko · 2018-08-13T13:22:45Z

phoneApp/js/app.js

+					name: "Дарий",
+					lastName: "Смирнов",
+					email: "DariySmirnov@ec.ua"
+				},


I guess such mock data no need anymore because you download them from the server ?

OlegLustenko · 2018-08-13T13:23:07Z

phoneApp/js/keypad.js

+      let insertedNumbersArr = display.innerHTML.split("");
      if (insertedNumbersArr.length > 0) {
-        let numberWithoutLast = insertedNumbersArr.slice(0,-1).join('');
+        let numberWithoutLast = insertedNumbersArr.slice(0, -1).join("");


please you const instead of let

OlegLustenko · 2018-08-13T13:25:20Z

phoneApp/js/router.js

+			let target =
+				e &&
+				e.target &&
+				(e.target.closest("a") || e.target.classList.contains("tab")); //You can add additional attributes


I'm pretty sure you don't need such event delegation because of your event working on a small area of responsibilities.

function switchRounter, should only change the router and don't make any checks and probably has a minimal of conditionalsa

viskosa added 8 commits July 12, 2018 12:36

phoneApp

4b321a5

start working with phoneapp

868b14d

it seems a phonebook is done

314b6a1

rendering of contacts page was done

bc2c99e

folders were deleted

6228af2

homeworks & classworks folders were returned

7ea8c7a

phoneApp was moved

5d8cc42

contacts page was virtualized

f874edd

OlegLustenko requested changes Jul 22, 2018

View reviewed changes

footer was make like a static content

84f468d

keypad.js was done

ce0a090

OlegLustenko requested changes Aug 3, 2018

View reviewed changes

viskosa added 2 commits August 5, 2018 14:40

edit-contact, user, add-user pages were virtualized

8a17850

refactoring

7041908

OlegLustenko requested changes Aug 10, 2018

View reviewed changes

viskosa added 2 commits August 10, 2018 17:08

refactoring v.2

37e75e9

try to make request

390e16a

OlegLustenko requested changes Aug 13, 2018

View reviewed changes

viskosa added 5 commits August 15, 2018 17:35

some changes

b3debda

adding user was done

9f21e51

opportuniry to edit & delete user added

1b3876b

webpack was added

7309c63

phone number was formatted, trying to configure webpack

f567994



		switchRouter() {
		const parent = document.querySelector('.main-nav');

Conversation

viskosa commented Jul 20, 2018

Uh oh!

viskosa commented Jul 21, 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!

viskosa commented Jul 23, 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!

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!

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!

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