Phone book #23
Phone book #23
Conversation
…g data from server.
… render name and phone from data recieved from server
|
|
||
| <script src="src/users.js"></script> | ||
| <script src="src/router.js"></script> | ||
| <script src="src/server-api.js"></script> | ||
| <!-- <script src="src/contacts.js"></script> --> |
There was a problem hiding this comment.
it's seems like an a duplication with line 20, please remove it
| <script src="src/keypad.js"></script> | ||
| <script src="src/edit-user.js"></script> | ||
| <script src="src/user.js"></script> | ||
| <!-- <script src="src/add-user.js"></script> --> |
There was a problem hiding this comment.
what the status of that script? it's better to remove commented code. The commented code just makes discouraged the next developer who inspect such code
| const fs = require('fs'); | ||
| const port = 3000; | ||
|
|
||
| http.createServer( (request, response) =>{ |
There was a problem hiding this comment.
ideally, you need your own server only for development purpose, so you could use some instruments like webpack-dev-server or so
| class App { | ||
| constructor(appContainerId) { | ||
| this.state = {}; | ||
| this.appContainer = document.getElementById(appContainerId); |
There was a problem hiding this comment.
appContainerId - not sure that variable should be dynamic, it's fine to make it static
|
|
||
| renderAppContainer(){ | ||
| return ` | ||
| ${this.renderAppHead()} |
There was a problem hiding this comment.
it's safe to store all HTML content inside the current method, no need additional abstraction.
I've been talked about additional methods when we've been used document.createElement in such situation abstraction could tell us more. But if you have just a plain HTML, no need an additional abstraction
| const serverAPI = this.app.serverAPI; | ||
| const updatingUser = serverAPI.updateUser(data); | ||
| updatingUser.then( response => { | ||
| this.app.pages.contacts.pageObject.users = []; |
There was a problem hiding this comment.
it's breaking single responsibility principal an could causes unpredictable errors in future
| const serverAPI = this.app.serverAPI; | ||
| const addingUser = serverAPI.addUser(data); | ||
| addingUser.then( response => { | ||
| return response.json(); |
| return response.json(); | ||
| }) | ||
| .then(addedUser => { | ||
| this.app.pages.contacts.pageObject.users = []; |
There was a problem hiding this comment.
it's breaking single responsibility principal an could causes unpredictable errors in future
| const serverAPI = this.app.serverAPI; | ||
| const deletingUser = serverAPI.deleteUser(id); | ||
| deletingUser.then( response => { | ||
| this.app.pages.contacts.pageObject.users = []; |
There was a problem hiding this comment.
it looks like really mess.
I believed pageObject is required overhead to structure, but it just makes the code more confusing.
If you want to store something you have to move its state and work with it directly.
If you want to update state - make 1 method where all updates would happens.
You could desing your app state withholding in mind 1 idea "I need to log my new state in console every time something changing"
| const newPage = 'contacts'; | ||
| const newState = { activePage : newPage }; | ||
| this.app.updateState(newState); | ||
| this.app.router.gotToPage(this.app.state); |
There was a problem hiding this comment.
well as I see it's not bad at all, but with problem in architecture
Основные функции работают.
Из того что я знаю точно не работает:
Вобще получилось очень сумбурно и я уверен что архитектур у к черту неправильная, но как его все связать правильно я не знаю...
MVC скорее всего тоже не получится.