js-dom homework by LuckyDnepr #145
Conversation
Need review...
| @@ -0,0 +1,84 @@ | |||
| "use strict"; | |||
|
|
|||
| import { readJSON } from "./readJSON.js"; | |||
There was a problem hiding this comment.
It would be better to just store the data from the first call to the JSON somewhere and use it, instead of reaching out to this function every time someone changes a tab.
|
|
||
| import { readJSON } from "./readJSON.js"; | ||
|
|
||
| const dataBaseUrl = "./data-films.json"; |
There was a problem hiding this comment.
Also, you already have the same string in the main-script.js file. Maybe consider moving it to the readJSON.js file. Though, if you remove readJSON function usage here, maybe you don't need readJSON.js file at all and you can keep this function in the main-script.js.
| const data = await readJSON(dataBaseUrl); | ||
| container.innerHTML = ""; | ||
| const film = data.find((film) => film.ID === +id); | ||
| container.appendChild(makeTitle(film)); |
There was a problem hiding this comment.
That's a lot of reaching out to DOM just to switch content on one action. Consider wrapping all these elements with a document fragment and using only one appendChild.
| asideMenuList.appendChild(makeAsideMenuItem(film.ID, film.title)); | ||
| }); | ||
| container.appendChild(asideMenuList); | ||
| return asideMenu; |
There was a problem hiding this comment.
This looks a little confusing. The purpose of this function is to render an AsideMenu. When you name function makeAsideMenu it usually means that it should return some kind of element, and this function does do that, but you're not using this element in any way, because what it also does is render the element on line 23 right away. Either rename the function to something like renderAsideMenu and remove the return or keep the return, remove rendering here and render somewhere outside this function using the returned value.
Add favicon. (^-^) It would be better to just store the data from the first call to the JSON somewhere and use it, instead of reaching out to this function every time someone changes a tab. kottans/frontend-2022-homeworks#145 (comment) Resolved Also, you already have the same string in the main-script.js file. Maybe consider moving it to the readJSON.js file. Though, if you remove readJSON function usage here, maybe you don't need readJSON.js file at all and you can keep this function in the main-script.js. kottans/frontend-2022-homeworks#145 (comment) Resolved That's a lot of reaching out to DOM just to switch content on one action. Consider wrapping all these elements with a document fragment and using only one appendChild. kottans/frontend-2022-homeworks#145 (comment) Resolved This looks a little confusing. The purpose of this function is to render an AsideMenu. When you name function makeAsideMenu it usually means that it should return some kind of element, and this function does do that, but you're not using this element in any way, because what it also does is render the element on line 23 right away. Either rename the function to something like renderAsideMenu and remove the return or keep the return, remove rendering here and render somewhere outside this function using the returned value. kottans/frontend-2022-homeworks#145 (comment) Resolved
Add favicon. (^-^) It would be better to just store the data from the first call to the JSON somewhere and use it, instead of reaching out to this function every time someone changes a tab. #145 (comment) Resolved Also, you already have the same string in the main-script.js file. Maybe consider moving it to the readJSON.js file. Though, if you remove readJSON function usage here, maybe you don't need readJSON.js file at all and you can keep this function in the main-script.js. #145 (comment) Resolved That's a lot of reaching out to DOM just to switch content on one action. Consider wrapping all these elements with a document fragment and using only one appendChild. #145 (comment) Resolved This looks a little confusing. The purpose of this function is to render an AsideMenu. When you name function makeAsideMenu it usually means that it should return some kind of element, and this function does do that, but you're not using this element in any way, because what it also does is render the element on line 23 right away. Either rename the function to something like renderAsideMenu and remove the return or keep the return, remove rendering here and render somewhere outside this function using the returned value. #145 (comment) Resolved
|
Thanks for review. |
|
Good job! |
js-dom-task
Demo |
Code base
The code is submitted in a dedicated feature branch.
Only code files are submitted.
Please, review.