Skip to content

phoneApp с помощью innerHtml - #17

Open
olgakuzminagithub wants to merge 19 commits into
masterfrom
phoneApp
Open

phoneApp с помощью innerHtml#17
olgakuzminagithub wants to merge 19 commits into
masterfrom
phoneApp

Conversation

@olgakuzminagithub

Copy link
Copy Markdown
Collaborator

No description provided.

Comment threadphoneApp/js/main.js Outdated
${this.colums.map(colum => {return`<th>${colum}</th>`}).join('')}
</thead>
<tbody>
${this.users.map(user => {return`

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you can omit return word it will look more harmonious

Comment threadphoneApp/js/main.js Outdated
</form>
<table class="table table-hover contacts">
<thead>
${this.colums.map(colum => {return`<th>${colum}</th>`}).join('')}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the same about return keyword

Comment threadphoneApp/js/main.js Outdated
<footer class="footer">
<div class="container bottom-radius">
<nav class="main-nav">
<a href="index.html" class="tab active">

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

<navclass="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

Comment threadphoneApp/js/main.js Outdated
},
];
const colums = ['Name', 'LastName', 'Email'];
const app = {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please make app a class with ContactPage name

Comment threadphoneApp/js/main.js Outdated
users,
colums,
render() {
document.body.innerHTML += this.renderHeader() +this.renderMain() + this.renderFooter();

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

const contactPageLayot = this.renderHeader() +this.renderMain() + this.renderFooter();

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,

<divid="mountNode"></div>

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

Comment threadphoneApp/js/keypad.js Outdated

}
render() {
const mountNode = document.createElement('div');

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't need to create such elements in JS :) add it to html file is fine

Comment threadphoneApp/js/keypad.js Outdated
render() {
const mountNode = document.createElement('div');
mountNode.setAttribute('id', 'mountNode');
mountNode.innerHTML += this.renderHeader() +this.renderMain() + this.renderFooter();

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you know... if we just could add "html-layout" without method dividings it could be more informative that
renderHeader + remderMain + renderFooter

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@olgakuzminagithub@OlegLustenko