-
Notifications
You must be signed in to change notification settings - Fork 47
Time - Angela #34
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 - Angela #34
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...?)
SummaryReally nice work, you hit the learning goals here. A good start to Rails. The only suggestion I can make is that you could combine your mark complete and incomplete methods into one method which toggles between them. |
| def mark_incomplete | ||
| @task = Task.find_by(id: params[:id]) | ||
| return redirect_to :task if @task.nil? | ||
| @task.update(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.
You should probably have an if statement here.
| def mark_complete | ||
| @task = Task.find_by(id: params[:id]) | ||
| return redirect_to :task if @task.nil? | ||
| @task.update(completed_at: Time.now.strftime("%d/%m/%Y")) | ||
| return redirect_to :tasks | ||
| 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.
Could you combine the mark_complete and mark_incomplete methods into a toggle_complate method that toggles between complete and incomplete?
|
|
||
| def destroy | ||
| @task = Task.find_by(id: params[:id]) | ||
| return redirect_to tasks_path if @task.nil? |
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 works, but you could also provide a 404 response here.
| <%= stylesheet_link_tag 'application', media: 'all', 'data-turbolinks-track': 'reload' %> | ||
| <%= javascript_pack_tag 'application', 'data-turbolinks-track': 'reload' %> | ||
| </head> | ||
|
|
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 would strongly suggest placing navigation links for your site in this layout file. This helps dry up layout code and provides a uniform interface.
| @@ -0,0 +1,12 @@ | |||
| <%= form_with model: @task, class:'create-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.
Nice, a partial view!
| @@ -0,0 +1,11 @@ | |||
| <h1>Tasks</h1> | |||
| <button><%= link_to "add new task", new_task_path %></button> | |||
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.
instead of using a button tag, you could use button_to. It functions like link_to but makes a mini-form and makes a post request by default.
https://apidock.com/rails/ActionView/Helpers/UrlHelper/button_to
| resources :tasks do | ||
| patch 'mark_incomplete', on: :member | ||
| patch 'mark_complete', on: :member | ||
| 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.
Nice use of resources!
Task List
Congratulations! You're submitting your assignment!
Comprehension Questions