-
Notifications
You must be signed in to change notification settings - Fork 49
Ports - Faiza #41
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 - Faiza #41
Conversation
…e of said column from String to Boolean, added toggle button, added some styles
…ssengers and rendered it in New and Edit views
Task ListWhat We're Looking For
Hi Faiza, good job with the beginning functionality of your project. It follows the conventions of RESTful routing, CRUD, and best practices for Rails as we expected and outlined. However, your interpretation of the "Task Complete" functionality is extremely not in the correct direction, and I want to address it. There are a lot of parts of your approach to the feature that concern me. I'd love to be able to chat some time about the following things:
Let's chat about this sometime in a one-on-one. I'll talk with you next week. Good job with getting the Rails part of this project done well and done to standard. However, your approach to the custom logic concerns me. |
| class TasksController < ApplicationController | ||
|
|
||
| def index | ||
| @tasks = Task.all.order(: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.
Nice attention to detail by ordering
app/controllers/tasks_controller.rb
Outdated
| def new | ||
| @task = Task.new | ||
| @task.task = "I have to... " | ||
| @task.completed = "Incomplete" |
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 data type should @task.completed be? Right here you set it to a string ("Incomplete"). What logic does the task need to do in order to make task completion functional?
app/controllers/tasks_controller.rb
Outdated
| def complete | ||
| task = Task.find_by(id: params[:id]) | ||
|
|
||
| task.completed? |
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.
The line task.completed? does nothing -- It just returns a boolean and then does nothing with it. Usually, we pair booleans with conditionals. What were you intending to do with this line of code?
| task = Task.find_by(id: params[:id]) | ||
|
|
||
| task.completed? | ||
| task.toggle(:completed) |
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.
Why would you always toggle this boolean?
app/views/tasks/index.html.erb
Outdated
| Completion status: <%= todo_item.completed %><br/> | ||
| <%= link_to todo_item.task, task_path(todo_item.id) %>, | ||
| <%= button_to "Mark Complete", {action: "complete", id: todo_item.id}, method: :patch, class: "complete-button" %> | ||
| <%= button_to "Mark Incomplete", {action: "complete", id: todo_item.id}, method: :patch, class: "incomplete-button" %><br/> |
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.
Both the "Mark Complete" and "Mark Incomplete" buttons go to the same controller action, which both toggle the completeness of the task. What is the effect? What happens when you manually test this feature out?
Task List
Congratulations! You're submitting your assignment!
Comprehension Questions
How are Rails migrations related to Rails models? | Migrations can be used to change the structure of the database.
Describe one area of Rails that are still unclear on | The route patterns that apparently return a response even though they don't lead to anything.