-
Notifications
You must be signed in to change notification settings - Fork 47
Space - Katie #39
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?
Space - Katie #39
Conversation
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...?)
SummaryNicely done, you hit the learning goals here. You've definitely demonstrated an understanding of the lessons we modeled in Ada Books. Good luck with Rideshare! |
| @@ -0,0 +1,20 @@ | |||
| Rails.application.routes.draw do | |||
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.
👍
| <h1>Welcome to Tasks List</h1> | ||
| <nav> | ||
| <ul> | ||
| <li><%= link_to "Create a new task", new_task_path %></li> | ||
| <li><%= link_to "View all tasks", tasks_path %></li> | ||
|
|
||
| </ul> | ||
| </nav> | ||
| </header> |
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.
I like this, but it should be the <body> tag.
| @@ -0,0 +1,11 @@ | |||
| <%= form_with model: @task, class: 'edit-task' do |f| %> | |||
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.
I realize this was reading after Task-List was due, but in future projects you can dry things up like this form with a partial view. Just a reminder.
| <ul> | ||
| <%@tasks.each do |task|%> | ||
| <li> | ||
| <%= link_to task.name, task_path(task) %> <%= button_to "Delete Task", task_path(task), method: :delete, data: {confirm: "Are you sure?"}%> <%= button_to "Complete Task", task_complete_path(task), 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.
This is a really long line, maybe break this up over multiple lines.
| redirect_to task_path(@task.id) | ||
| return | ||
| else | ||
| render :new, :bad_request # show the new task form view again |
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.
I just find this more readable, but I think yours is correct too.
| render :new, :bad_request # show the new task form view again | |
| render :new, status: :bad_request # show the new task form view again |
| redirect_to task_path(@task.id) | ||
| return | ||
| else | ||
| render :edit |
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.
| render :edit | |
| render :edit, status: :bad_request |
| redirect_to task_path(@task.id) | ||
| return | ||
| else | ||
| render :edit |
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 render edit here? Maybe instead just redirect them someplace.
Task List
Congratulations! You're submitting your assignment!
Comprehension Questions