-
Notifications
You must be signed in to change notification settings - Fork 49
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
Ports - Heather #32
base: master
Are you sure you want to change the base?
Ports - Heather #32
Conversation
Task ListWhat We're Looking For
Well done on this project, Heather! Your project hits all the things I was looking for: following Rails best practices, RESTful routing, and CRUD. The places of improvement that I have noted are:
There are also just a few other comments I have Also, you used some of the live code copy (like "MAKE! THAT! task!"), so don't forget to change small details like that to make this app your own :) :) :) (Not a big deal, just something that might feel nice for yourself) That being said, your project looks pretty good. Great work! |
{ name: "cleaning", description: "vacuum" }, | ||
{ name: "dinner", description: "make soup" }, | ||
{ name: "shopping", description: "Go to Target" }, | ||
] |
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.
Don't forget to refactor your code often, so you remember to delete unused variables like this!
task = Task.new( | ||
name: params["task"]["name"], | ||
description: params["task"]["description"], | ||
) |
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.
This is a place where you can refactor this to use task_params
instead of outlining name
and description
end | ||
|
||
def edit | ||
puts params |
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.
You'd probably want to delete your puts
!
end | ||
|
||
def update | ||
task = Task.find_by(id: params["id"]) |
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.
Strong params!
task = Task.find_by(id: params[:id]) | ||
task.toggle(:completed) | ||
task.save | ||
task.touch |
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.
What does this line do? Your tests still pass when I remove this line, and also the app still seems to work
<%else%> | ||
<%= button_to "Mark Incomplete", mark_complete_path(task.id), method: :patch, class: "status-button" %> | ||
|
||
<%end %> |
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.
this view and the logic in it looks good! You may want to mind your indentation though
Task List
Congratulations! You're submitting your assignment!
Comprehension Questions