-
-
Notifications
You must be signed in to change notification settings - Fork 184
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
Andrey action menu #265
Andrey action menu #265
Conversation
Please read bot's explanations and instructions carefully. Feel free to ask questions. Students' chat will be very helpful in resolving this issue. |
A risky approach. I am not sure if it won't delete files from the PopUp task. |
Hey! Congratulations on your PR! 😎😎😎 Let's do some self-checks to fix most common issues and to make some improvements to the code before reviewers put their hands on the code. Go through the requirements/most common mistakes listed/linked below and fix the code as appropriate. If you have any questions to requirements/common mistakes feel free asking them here or in Students' chat. When you genuinely believe you are done put a comment stating that you have completed self-checks and fixed code accordingly. Also, be aware, that if you would silently ignore this recommendation, a mentor can think that you are still working on fixes. And your PR will not be reviewed. 😒 Please, make sure you haven't made common mistakes Universal recommendations:
Also take a note of the requirements above and follow them in all your future projects. By the way, you may proceed to the next task before this one is reviewed and merged. Sincerely yours, |
I understand that I have to delete unnecessary files from the "change file", but I don't understand how to do it( |
Oh, I see... Your PopUp wasn't in its own dedicated folder, so you needed to move it. And you did this in a single commit. You cannot really split a commit. They are atomic. Learning: commit frequently and never do "big things" in one commit. Merging commits is easier than splitting them. Let's fix this in two steps. Step 1. Renaming files. I'll let you know what to do next. |
Thanks! I opened a new PR |
#266 merged. If any step fails, post an error message here. If in doubt ask questions. |
to force push I have to go to the |
Yes |
5084a1b
to
c716310
Compare
it worked) thank you! |
Hey! Congratulations on your PR! 😎😎😎 Let's do some self-checks to fix most common issues and to make some improvements to the code before reviewers put their hands on the code. Go through the requirements/most common mistakes listed/linked below and fix the code as appropriate. If you have any questions to requirements/common mistakes feel free asking them here or in Students' chat. When you genuinely believe you are done put a comment stating that you have completed self-checks and fixed code accordingly. Also, be aware, that if you would silently ignore this recommendation, a mentor can think that you are still working on fixes. And your PR will not be reviewed. 😒 Please, make sure you haven't made common mistakes Universal recommendations:
Also take a note of the requirements above and follow them in all your future projects. By the way, you may proceed to the next task before this one is reviewed and merged. Sincerely yours, |
I checked the work and fixed a bug. Please review my work |
This issue has been automatically marked as stale because there were no activity during last 14 days. It will be closed in 7 days if no further activity occurs. Thank you for your contributions. А. Чому так? Б. Що робити, якщо в піарі нема оновлень, оскільки не зрозуміло, що треба зробити? В. А якщо я все зробив(ла) і це ментор не рев'юває мої зміни?
Г. Хіба недостатньо того, що я додав(ла) коміт із змінами? Традиційна пропозиція: задай питання по вищенаписаному в студентському чаті. |
A proposal: post a link to your app demo and source code in Students' chat and ask peers for review. P.S. Also this my comment should remove |
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.
Good work!
Thanks! I'll try to fix |
I have corrected the errors. But not sure if I used the append method correctly |
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.
Good progress!
const main = document.querySelector(".main"); | ||
const menuItems = (item, menuList) => { | ||
item.map(function (item) { | ||
let sideWrap = document.createElement('div'); |
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.
div
can't be a child element of the ul
for this case use createFragment
menuList.append(sideWrap) | ||
}); | ||
}; | ||
const content = (i = 1, arr = companies) => { |
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.
Functions denote actions hence their names should start with a verb. Variables containing strings, numbers, and objects are normally nouns. Boolean variables/functions start with is, does, has etc. A variable containing multiple entities and functions returning lists containing entity names in plural form.
Thank you! I didn't know about |
I did it with |
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.
Good progress!
item.map(function (item) { | ||
let sideWrap = document.createElement('template'); | ||
sideWrap.innerHTML = `<li class="sidebar__menu__item"><button class="sidebar__btn" data-company = ${item.id}>${item.title}</button></li>`; | ||
menuList.append(sideWrap.content); |
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.
Adding elements to DOM from a loop is a bad practice. A browser will run reflow and repaint for every element in the loop. Instead, you can:
- Use append method, which can add several elements in one operation.
- Create some wrapper, add your items to the wrapper and then add it to DOM. It will be one operation.
- Clone current container. Add items to a container and then replace your old container with a new one. But be aware of event listeners.
- Use innerHTML instead.
]; | ||
const menu = document.querySelector(".sidebar__menu"); | ||
const main = document.querySelector(".main"); | ||
const addmenuItems = (item, menuList) => { |
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.
What's the point to use menuList
parameter if you use this function once, and in this parameter, you pass menu
element from line 47?
`; | ||
}; | ||
const changeContent = (e) => { | ||
let btn = e.target; |
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.
Use const
for elements that will not be changed.
menuList.append(sideWrap.content); | ||
}); | ||
}; | ||
const addcontent = (i = 1, arr = companies) => { |
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.
Why do you add a second parameter if you do not pass him in your code?
This issue has been automatically marked as stale because there were no activity during last 14 days. It will be closed in 7 days if no further activity occurs. Thank you for your contributions. А. Чому так? Б. Що робити, якщо в піарі нема оновлень, оскільки не зрозуміло, що треба зробити? В. А якщо я все зробив(ла) і це ментор не рев'юває мої зміни?
Г. Хіба недостатньо того, що я додав(ла) коміт із змінами? Традиційна пропозиція: задай питання по вищенаписаному в студентському чаті. |
action-menu
Demo |
Code base
The code is submitted in a dedicated feature branch.
Only code files are submitted.
Please, review.