-
Notifications
You must be signed in to change notification settings - Fork 1
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
JS Capstone Project PR #36
Conversation
Initial setup
Header footer setup
display list of items on the home page done- Student A
Like feature
reservation counter test
Hi, we are facing a webhit error while making the pull request here, however, only a warning and hints but no error locally. And our app is also working fine. We tried to resolve this issue for the last 24 hours but with no luck and it would be great if we can have some insight on that, please. Please find the screenshot below. Thank you. |
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.
STATUS: CHANGES REQUIRED ♻️♻️
Hello @porag-m06, @ghermaico135 and @maryam0007 👋🏽
Good job working on the project so far!
To highlight 👍🏽
✔️ App is well designed
✔️ Tests pass
✔️ Involvement API is used
However, there are some issues that you need to work on to go to the next project but you are almost there!
Required Changes ♻️
_Check the comments under the review 👇🏽 _
Optional suggestions
Every comment with the [OPTIONAL] prefix is not crucial enough to stop the approval of this PR. However, I strongly recommend you to take them into account as they can make your code better.
- You can consider closing the issues that have already been fixed.
Cheers and Happy coding!👏👏👏
Feel free to leave any questions or comments in the PR thread if something is not 100% clear.
Please, remember to tag me in your question so I can receive the notification.
Please, do not open a new Pull Request for re-reviews. You should use the same Pull Request submitted for the first review, either valid or invalid unless it is requested otherwise.
As described in the Code reviews limits policy you have a limited number of reviews per project (check the exact number in your Dashboard). If you think that the code review was not fair, you can request a second opinion using this form.
src/index.js
Outdated
/* eslint-disable */ | ||
console.error(error.message); | ||
/* eslint-enable */ | ||
} |
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.
- Kindly do not disable linters as they help us write cleaner code.
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.
Hello, @Ghiftee thank you so much for the detailed review, however, could you please suggest what could be an alternative here to get the message in case an error occurs? [in this particular case inside a catch block]
src/index.js
Outdated
/* eslint-disable */ | ||
console.log("No data found while fetching for meal list!!!"); | ||
/* eslint-enable */ |
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.
- Kindly do not disbale linters as they help us write cleaner code
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.
Sure, will be removing that. Thank you
<h4><span id = "s${foodArray[index].idMeal}">0</span> likes</h4> | ||
</div> | ||
<div class="flex crd-btns"> | ||
<button class="comment">Comments</button> |
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.
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.
Hello, again, So, in this case we were only 2 people (@ghermaico135 as Student C and me @porag-m06 - as Student A) on the team. Then @maryam0007 as student B Joined. And we divided and assigned our tasks according to the instructions.
Implementation of the Comment section was assigned on @maryam0007, however, unfortunately, due to her illness she could not complete any of the assigned tasks according to the instructions.
Therefore, that part is unfinished. In this scenario is mandatory for us 2 (As students A and C ) to complete the part of Student B as well to get approved? It would be great if you could shed some light on it, please.
As, technically we are only 2 people team. Thanks
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.
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.
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.
- As suggested by the previous reviewer, please implement the comments section so that users can post comments. It's so unfortunate that Student B fell sick but when such a thing occurs, you need to contact the Student Success team and forge a way forward together. Deciding on a course of action individually is not professional and could lead to disastrous results on a job. Please reach out to The Student Success Team at your earliest convenience. 👍🏾
src/modules/reservation.js
Outdated
/* eslint-disable */ | ||
console.error(error.message); | ||
/* eslint-enable */ |
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.
- Kindly do not disable linters as they help us write cleaner code.
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.
please suggest what could be an alternative here to get the message in case an error occurs? [in this particular case inside a catch block]
Or, should we remove it any ways? Thanks
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.
- If the user needs to see the error message, you can display it on the webpage, style it and reword it so that it's not technical and a user can understand what the problem is and what they should do. If the user does not need to see it, you should handle it in code so that the application does not crash. logging the error to the console can leak confidential information to hackers and users do not always check the console when using web apps. 😄
src/modules/reservation.js
Outdated
}, 1000); | ||
}, 1000); | ||
}); | ||
};// displayMeals |
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.
- Please do not leave commented code that are not descriptive in your codebase.
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.
Sure, will be removing that. thanks
src/style.css
Outdated
padding: 30px; | ||
display: block; | ||
|
||
/* visibility: hidden; */ |
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.
- Please do not leave commented code that is not descriptive in your codebase, this would give a cleaner and more professional code.
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.
Sure, will be removing that. Thank you
src/style.css
Outdated
margin-top: 1rem; | ||
margin-left: 1.3rem; | ||
|
||
/* background-color: #fff; */ |
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.
- Please do not leave commented code that is not descriptive in your codebase, this would give a cleaner and more professional code.
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.
Sure, will be removing that. Thank you
src/style.css
Outdated
.crd-btns { | ||
gap: 17px; | ||
} |
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.
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.
Thank you so much for the kind review, however, just to be sure as part of following the medium fidelity wireframe, can't the implementation be improvised a bit?
Thanks
} | ||
|
||
footer h3 { | ||
text-align: center; |
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.
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.
can't the implementation be improvised according the to design theme?
Thanks
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.
- [ OPTIONAL] Please note that the footer text is left aligned. It would be better if you did the same. 👍🏾
README.md
Outdated
- **Use of medium-fidelity wireframe attached below to create a UI.** | ||
<img src="./leaderboard_wireframe.png" alt="Leaderboard wireframe" width="auto" height="auto" /> | ||
- **Developed UI:** | ||
<img src="./leaderboard_Design.png" alt="Leaderboard wireframe" width="auto" height="auto" /> |
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.
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.
Sure, will be removing that. Thank you
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.
Hi @team!,
While you made a great effort in this project, unfortunately, I cannot proceed to review your code.
Invalid Code Review Request
You have submitted a project, that has got linter errors 😞
Your Code Review Request will be marked as invalid in your Dashboard, so please submit a new one once you are ready 🙏
Cheers and Happy coding!👏👏👏
Feel free to leave any questions or comments in the PR thread if something is not 100% clear.
Please, remember to tag me in your question so I can receive the notification.
Please, do not open a new Pull Request for re-reviews. You should use the same Pull Request submitted for the first review, either valid or invalid unless it is requested otherwise.
Invalid Code Review Request does not count into the code reviews limit.
Hi, @mwanawabangona We tried to resolve this issue for the last 24 hours but with no luck and it would be great if we can have some insight on that, please. Please find details in the below link that was mentioned in the previous review request, Thanks |
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.
Hi @porag-m06 & team,
Great work on making the changes requested by a previous reviewer 👏
You've done well implementing some of the requested changes, but there are still some which aren't been addressed yet.
Highlights
The app is well designed 👍🏾
Tests are passing 👍🏾
Involvement API is used 👍🏾
Required Changes ♻️
Check the comments under the review.
Optional suggestions
Every comment with the [OPTIONAL] prefix won't stop the approval of this PR. However, I strongly recommend you to take them into account as they can make your code better. Some of them were simply missed by the previous reviewer and addressing them will really improve your application.
Cheers and Happy coding!👏👏👏
Feel free to leave any questions or comments in the PR thread if something is not 100% clear.
Please, remember to tag me in your question so I can receive the notification.
Please, do not open a new Pull Request for re-reviews. You should use the same Pull Request submitted for the first review, either valid or invalid unless it is requested otherwise.
As described in the Code reviews limits policy you have a limited number of reviews per project (check the exact number in your Dashboard). If you think that the code review was not fair, you can request a second opinion using this form.
} | ||
|
||
footer h3 { | ||
text-align: center; |
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.
- [ OPTIONAL] Please note that the footer text is left aligned. It would be better if you did the same. 👍🏾
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.
Hi Team 👋🏾
Your project is complete! There is nothing else to say other than... it's time to merge it
Congratulations! 🎉
Optional suggestions
- please add a working link of your live demo to your readme file because when you code base I being used your readme file help visitor navigate through your project
Every comment with the [OPTIONAL] prefix won't stop the approval of this PR. However, I strongly recommend you to take them into account as they can make your code better. Some of them were simply missed by the previous reviewer and addressing them will really improve your application.
Cheers and Happy coding!👏👏👏
Feel free to leave any questions or comments in the PR thread if something is not 100% clear.
Please, remember to tag me in your question so I can receive the notification.
As described in the Code reviews limits policy you have a limited number of reviews per project (check the exact number in your Dashboard). If you think that the code review was not fair, you can request a second opinion using this form.
Hi, Please find the PR for review below,
Live Demo : The kanban Project
Presentation: Video
Highlights:
Used API for Sending and receiving data.
Used Base API fetching meal data (Meals DB: data about meals.)
Used Involvement API to record the different user interactions (e.g. likes, reservations).**
Used ES6 syntax.
Used ES6 modules.
Used callbacks and promises.
Used web pack.
Applied JavaScript best practices and language style guides in code.
Added unit tests.
Followed Gitflow.