DOM-api-task #442
DOM-api-task #442
Conversation
|
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've completed self-checks, did'nt find any common errors |
|
@Conversee12 Good work! 👍 |
| return rockBands.forEach((band) => { | ||
| const { title } = band; | ||
| const button = document.createElement("button"); | ||
| button.classList.add("buttons"); |
There was a problem hiding this comment.
Why class is buttons for a single button?
| <section class="container"> | ||
| <h2 class="section__title"></h2> | ||
| <div class="container__image-text"> | ||
| <img src="" alt="" class="section__img"> |
There was a problem hiding this comment.
img must have not emptyalt attribute.
|
|
||
| function showMain(target) { | ||
| const content = rockBands.filter( | ||
| (band) => target.innerText === band.title |
There was a problem hiding this comment.
Searching by title is a bad idea. Add ids.
| } | ||
|
|
||
| function showMain(target) { | ||
| const content = rockBands.filter( |
There was a problem hiding this comment.
Looks like find is more right choice in your situation.
| } | ||
|
|
||
| const sideBar = document.querySelector(".sideBar"); | ||
| initButtons(sideBar); |
There was a problem hiding this comment.
You use sideBar only inside initButtons and initButtons call only once. You can move sideBar into initButtons.
| body { | ||
| display: flex; | ||
| flex-direction: column; | ||
| background: #0b0e14; |
There was a problem hiding this comment.
Move colors to css3 vars.
|
Okey, i fixed it, but I didn't understand why (.find) is better here, please explain whether it still needs to be replaced. |
|
In |
| button.classList.add("button"); | ||
| button.id = id; | ||
| button.innerText = title; | ||
| button.addEventListener("click", ({ target }) => { |
There was a problem hiding this comment.
Can we use event bubbling for use single event handler?
There was a problem hiding this comment.
Yee, i finally understand the difference between filter and find, thanks
DOM-api-task
Demo |
Code base
The code is submitted in a dedicated feature branch.
Only code files are submitted.
Please, review.