Conversation
There was a problem hiding this comment.
STATUS: Required Changes ♻️ ♻️
###Hi @aster-alemu 👋 Im @MosDevx . Your Reviwer for this Pull Request
Great Job so far 👍 👍
There are some few issues you still need to work on before you can merge.
Highlights 🏅 🏅
- Great work using the right GitHub Flow ✔️
- No Linter errors ✔️
- Code is well organized with good folder structure ✔️
Required Changes ♻️
Kindly sort out the issues highlighted below 👍
| // //! My Own index js | ||
| // import './style.css'; | ||
| // // import './css/pop-window.css' | ||
|
|
||
| // import popupWindow from './modules/commentsPopupWindow.js'; | ||
| // import getMovies from './modules/getMovies.js'; | ||
| // import { | ||
| // getComments, | ||
|
|
||
| // } from './modules/commentsApi.js'; | ||
|
|
||
| // const popupContainer = document.getElementById('popup-container'); | ||
|
|
||
| // const movieArray = await getMovies(); | ||
|
|
||
| // const allIdsArray = []; | ||
| // movieArray.forEach((movie) => { | ||
| // allIdsArray.push(movie.showId); | ||
| // }); | ||
|
|
||
| // async function getAllComments(showIdsArray) { | ||
| // const allCommentsArray = []; | ||
|
|
||
| // let allArray = []; | ||
| // for (let index = 0; index < showIdsArray.length; index += 1) { | ||
| // const comment = getComments(showIdsArray[index]); | ||
| // allCommentsArray.push(comment); | ||
| // } | ||
| // allArray = await Promise.all(allCommentsArray); | ||
|
|
||
| // return allArray; | ||
| // } | ||
|
|
||
| // const commentsArray = await getAllComments(allIdsArray); | ||
|
|
||
| // function closeModal() { | ||
| // popupContainer.innerHTML = ''; | ||
| // } | ||
|
|
||
| // const popup = popupWindow(movieArray[2], commentsArray[2], closeModal); | ||
|
|
||
| // popupContainer.append(popup); No newline at end of file |
There was a problem hiding this comment.
Please remove all commented code from you codebase. This ensures readability and clean code practices. There will be less chance of hard to trace errors in the future
| "test": "jest --env=jsdom" | ||
| }, | ||
| "dependencies": { | ||
| "dotenv-webpack": "^8.0.1", |
There was a problem hiding this comment.
Kindly remove all packages you are not using from you project {dotenv-webpack}
This will enable faster download and installation in the future.
It also reduces the chances of depending on packages that are poorly maintained and which will thus introduce security holes.
| "jest": "^29.3.1", | ||
| "jest-environment-jsdom": "^29.3.1" | ||
| "jest-environment-jsdom": "^29.3.1", | ||
| "node-vibrant": "^3.1.6" |
There was a problem hiding this comment.
Kindly remove all packages you are not using from you project. { node-vibrant}
This will enable faster download and installation in the future
It also reduces the chances of depending on packages that are poorly maintained and which will thus introduce security holes.
MosDevx
left a comment
MosDevx
left a comment
There was a problem hiding this comment.
Status Aprroved 🥇 🥇
To Highlight 💪🏻
You've done a great job implementing the changes.
Great job man! 👏 There is nothing to say except to merge it!
Happy coding!!!!
2 participants
In this PR, I have been doing the following activities.