-
Notifications
You must be signed in to change notification settings - Fork 47
Time - Haben #26
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?
Time - Haben #26
Conversation
beccaelenzil
left a comment
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.
Great work on this project! You're off to a great start with rails and it's clear the learning goals were met. Nice work incorporating strong_params and resources. Keep up the hard work!
|
|
||
| end | ||
|
|
||
| def completed_at |
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.
Consider naming this controller action something like mark_complete to avoid confusion between the completed_at attribute and the completed_at action.
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.
In addition, consider how making two separate methods, for instance mark_complete and mark_incomplete would make this action idempotent.
| <div class="buttons"> | ||
| <%= button_to "Edit Task", edit_task_path(task[:id]), method: :get %> | ||
| <%= button_to "Delete Task", task_path(task[:id]), method: :delete, data: {confirm: "Are you sure you want to Delete?"} %> | ||
| <%= button_to "#{task.completed_at ? "Completed Task" : "Task Not Completed"}", completed_task_path(task[:id]), method: :patch %> |
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 use of a ternary for conditionally showing different text based on the state of the task! Consider naming these buttons something like "Mark Complete" and "Unmark Complete" to add clarity to the user exeperience.
| @@ -0,0 +1,15 @@ | |||
| <div> | |||
| <h3 class="index_title">Tasks to be Completed</h3> | |||
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.
Minor note: this title is a little misleading because you include all tasks (completed and not) in the list.
| // You can use Sass (SCSS) here: https://sass-lang.com/ | ||
| @import "bootstrap"; | ||
|
|
||
| // @import "**/*"; |
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 work adding styling! Minor note: before you have any tasks, some of your links overlap with eachother.
Task ListMajor Learning Goals/Code Review
Functional Requirements/Manual Testing
Overall Feedback
Code Style Bonus AwardsWas the code particularly impressive in code style for any of these reasons (or more...?)
|
Task List
Congratulations! You're submitting your assignment!
Comprehension Questions