-
Notifications
You must be signed in to change notification settings - Fork 0
Homework 15 #29
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
base: master
Are you sure you want to change the base?
Homework 15 #29
Conversation
| const uniqueLetters = new Set(); | ||
|
|
||
| turnStrToArray.forEach(letter => { | ||
| uniqueLetters.add(letter); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
А разве нет способа добавить в Set, проще?
|
|
||
| ]; | ||
|
|
||
| const footerTabs = [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's not the data, it just a static part of the content and it's not required to virtualize it
| 'Add user' | ||
| ]; | ||
|
|
||
| const idOfIcons = [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the same about icons
| return document.createElement(tagName); | ||
| }, | ||
|
|
||
| setClass(target, className) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like that decomposition with a lot of helpers.
I guess that methods should be moved to some other class or factory and make it like a helpers or so
| const nav = this.newElem('nav'); | ||
| this.setClass(nav, 'main-nav'); | ||
|
|
||
| footerTabs.forEach((tab, index) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please make footer is static
| const tabContent = this.newElem('span'); | ||
|
|
||
| this.setClass(link, 'tab'); | ||
| this.setClass(icon, `glyphicon glyphicon-${idOfIcons[index]}`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's not good to have a dependency to index
For future probably in such case, you should rename idOfIcons -> sortedIdOfIcons because of they depend on order
| this.createMainContent(); | ||
| this.createFooter(); | ||
|
|
||
| document.body.appendChild(this.header); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it will be better if you do not create a header inside such methods but return them.
It's not obvious if this.main mutating during another creator the same idea behind this.header and this.footer
| 'plus' | ||
| ]; | ||
|
|
||
| const contactPage = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you know it worth to make it as class and name it something like
class ContactsPage|
|
||
| const contentForThead = ['Name', 'Last Name', 'Email']; | ||
|
|
||
| const users = [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please move all logic relates to PhoneBook to another directory and work on it independently from home tasks
No description provided.