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

Recipe App πŸ‘¨β€πŸ’» πŸ‘¨β€πŸ’» #17

Merged
merged 156 commits into from
Feb 9, 2024
Merged

Conversation

tsheporamantso
Copy link
Owner

Recipe App πŸ‘¨β€πŸ’» πŸ‘¨β€πŸ’»

On this milestone we implemented the following

  • Login page and registration page:

    • Should be built with Devise.
  • Recipes list:

    • Should display a list of recipes created by the logged-in user as in the wireframe.
    • Should lead to recipe details.
  • Public recipe list:

    • Should display a list of all public recipes ordered by newest as in the wireframe.
    • Should lead to recipe details.
    • If the user is the owner of the recipe, should allow the user to delete it.
  • Recipe details:

    • Should display a toggle button that allows for a recipe to be made public or private.
    • If the recipe is public or the user is the owner of the recipe, should display the recipe details as in the wireframe.
    • If the user is the owner of the recipe, should lead to the form that allows the user to add new food.
  • Make sure there are no N+1 queries happening.

  • Create a navigation menu that allows users to open all of the pages you created.

  • Write unit and integration tests

  • Food list:

    • Should display a list of food added by the logged-in user as in the wireframe (display also quantity of a given food).
    • Should lead to a form that allows users to add new food.
  • General shopping list view:

    • Should show the list of food that is missing for all recipes of the logged-in user (compare the list of food for all recipes with the general food list of that user).
    • Should count the total food items and total price of the missing food.
  • Check and correct Rubocop offences

  • Check and correct Stylelint errors

Copy link

@Ibizugbe Ibizugbe left a comment

Choose a reason for hiding this comment

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

Hi @tsheporamantso @Fombi-Favour πŸ‘‹πŸΌπŸ‘‹πŸΌπŸ‘‹πŸΌ,

request change

Good job so far!
There are some issues that you still need to work on to go to the next project but you are almost there!

Highlightsβœ…βœ…βœ…

  • Account management is built using deviseπŸ‘πŸΌ.
  • Unit and integration tests have been written and all tests are passingπŸ‘πŸΌ.
  • n+1 concerns have been taken care ofπŸ‘πŸΌ.
  • Nice job implementing the toggle button for making a recipe publicπŸ‘πŸΌ.
  • github flow is usedπŸ‘πŸΌ.
  • Linters are passingπŸ‘πŸΌ.

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.

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.

</div>

<div class="btns">
<%= button_to "Generate Shopping List", "", class: "add-food-link"%>
Copy link

Choose a reason for hiding this comment

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

Nice job so farπŸ‘πŸΌ. I noticed that when the Generate shopping list button is clicked, an error is rendered which breaks the app, even though the shopping list is generated. Kindly fix this, as this can lead to a poor user experience. I suggest that when the button is clicked, it should lead the user to the general shopping list view so the user can see the result of his actionsπŸ˜‰.

image

Comment on lines 4 to 29
<%= form_with(model: @recipe, url:user_recipes_path(@user), local: true, method: :post) do |form| %>
<div>
<%= form.label :name %><br>
<%= form.text_field :name, autocomplete: 'off' %>
</div>

<div>
<%= form.label :description %><br>
<%= form.text_area :description %>
</div>

<div>
<%= form.label :preparation_time %><br>
<%= form.number_field :preparation_time, in: 1..1000, step: 1, autocomplete: "off" %>
</div>

<div>
<%= form.label :cooking_time %><br>
<%= form.number_field :cooking_time, in: 1..1000, step: 1, autocomplete: "off" %>
</div>

<div>
<%= form.label :public %><br>
<%= form.check_box :public %>
</div>

Copy link

Choose a reason for hiding this comment

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

I noticed you added validation to all your forms, which is a good practiceπŸ˜‰. The issue currently is that, for all your input fields or forms, whenever a user proceeds to submit with any empty field, it breaks the app. This can lead to a poor user experience. If a field is not provided, I suggest you send an error message to the user on the same page or you can use the required HTML attribute that prevents the user from submitting the form automaticallyπŸ˜‰.

In summary, let's find a way of preventing the user from submitting a form with empty fields, but without breaking the appπŸ‘πŸΌ. Please remember to implement this for all your formsπŸ˜‰.

image

Copy link

@AmaduKamara AmaduKamara left a comment

Choose a reason for hiding this comment

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

Status: Approved 🟒 🟒 🟒

Hi Team,

Good job working on the changes highlighted by the previous reviewer πŸ‘πŸ½

You guys did a great work. Keep it up πŸ‘πŸ½

Your project is complete! There is nothing else to say other than... it's time to merge it: ship it:
Congratulations! πŸŽ‰

Highlights:

βœ… You set up Gitflow correctly πŸ‘πŸ½
βœ… The Linter checks passed successfully πŸ‘πŸ½
βœ… Good descriptive PR and README file πŸ‘πŸ½
βœ… Good commit messages πŸ‘πŸ½
βœ… The app works fine and all features were implemented as expected πŸ‘πŸ½
βœ… Implemented tests and all tests passed successfully πŸ‘πŸ½

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.

@tsheporamantso tsheporamantso merged commit 02977a4 into main Feb 9, 2024
3 checks passed
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.

4 participants