Skip to content
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

Matalkbt code review2 #9

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Matalkbt code review2 #9

wants to merge 3 commits into from

Conversation

matalkbt
Copy link
Collaborator

@matalkbt matalkbt commented Nov 8, 2021

The application is looking great so far! I love seeing the progress. Only a few minor code changes from me.

…mports. It's important to use the proper annotations to identify the layer instead of using the general annotation.
… the logger to log errors. It's better practice to use the logger to log exceptions/errors. Added in the exception as reference to the error logged.
…ences the div a little lower. Just a nice feature for users to easily navigate to what they are looking for. Also, added a design recommendation for the shopping cart page.
Comment on lines +66 to +68
<!-- As a recommendation, you can have the 'add' button trigger a Modal to select the specifics for each type of food.
This helps prevent the unusual text display as seen here. This however is a design choice, and the course is more about
backend than front end development.-->

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Putting conversation/discussion in code comments is bad practice, as that clutters up the source code and does not belong in source code files. I mentioned that in the code review instructions. Discussion and comments are very good, but have that in the appropriate place, which is in the "Files Changed" tab in the pull request in GitHub (which is where I am writing this and you are reading this). That way, you can have a conversation with replies, right where the code is happening, and without cluttering up the source code itself.

@@ -54,7 +54,7 @@ <h4> A restaurant is a place where you can enjoy the food you like with your fam

<button class="btn btn-info"> Create Your Order </button>

<p3> <br>Don't have an account with us? Sign Up </p3>
<p3> <br>Don't have an account with us? <a href="#signUp">Sign Up</a> </p3>

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't a # href point to an a tag?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When we # it points to a corresponding id on the page. So it just jumps down the page to the already created div. It could point to an a tag. Is it better practice to do this?

@discospiff
Copy link

From this review, I'd harvest the logging. Logging is always a good idea.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants