-
Notifications
You must be signed in to change notification settings - Fork 49
Ports - Amy M. #25
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
base: master
Are you sure you want to change the base?
Ports - Amy M. #25
Conversation
Task ListWhat We're Looking For
Great work on this project, Amy! This totally meets all of the requirements that I was looking for: Rails best practices, RESTful routes, and CRUD operations. The code and the app look great, too. The tests look great! That being said, I have a few comments, on honestly they're all small places of improvement/me nitpicking. Another big comment I have is: You've included flash! I want to check-in and acknowledge that this is above and beyond our requirements. Our TaskList tests that we provided at first wrongfully had some tests on flash. Although we've removed them and updated everyone on Slack, you've kept them in -- I want to make sure you know that we didn't mean to accidentally make you use flash, we'll go over flash more in depth this week, and good job on using it in this project! Good work on this! |
| end | ||
|
|
||
| def show | ||
| task_id = params[:id].to_i |
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.
Actually, you don't need to do .to_i, and you can just do task_id = params[:id]! That's because Rails lets us get away with this.
| @task = Task.find_by(id: task_id) | ||
| if @task.nil? | ||
| flash[:error] = "Could not find task with id: #{task_id}" | ||
| redirect_to task_path |
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.
Actually, when you manually test this, because it redirects back to the task_path, which... is this action... and then it gets into this case again... which redirects back to the task_path... it gets into an infinite loop. You'll probably want to fix this by redirecting somewhere else
| <li> | ||
| <%if task.complete == false%> | ||
| <div> | ||
| <a href="/tasks/<%=task.id%>"> <%= task.name %>: <%= task.description %> Due:<%= task.completion_date %> </a> |
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.
Best pracitce would be to use the link_to helper, and also to reference the path with the path helper (here, task_path(task) instead of /tasks/...
| </div> | ||
| <%else%> | ||
| <div class="strike"> | ||
| <a href="/tasks/<%=task.id%>"> <%= task.name %>: <%= task.description %> Due:<%= task.completion_date %> </a> </div> |
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.
Same as above: Might be good to use link_to and the path helper
| </div> | ||
| <%else%> | ||
| <div class="strike"> | ||
| <a href="/tasks/<%=task.id%>"> <%= task.name %>: <%= task.description %> Due:<%= task.completion_date %> </a> </div> |
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.
indentation got kind of weird here... might want to put the ending </div> on the next line
Task List
Congratulations! You're submitting your assignment!
Comprehension Questions