Skip to content

Conversation

@nkiruka
Copy link

@nkiruka nkiruka commented Oct 1, 2017

Task List

Congratulations! You're submitting your assignment!

Comprehension Questions

Question Answer
Describe in your own words what the Model is doing in Rails Model: handles data and it is where the data resides for instance in a database. Model interacts with the controller and contains all the code related to the db in the model. Each row in the db table is an instance of the object (Is this true?).
Describe in your own words what the Controller is doing in Rails Controller: handles decision and acts like a "middle person" and interacts with the view and model. Controller contains the code involved in making decisions when a request is received the browser. Controller processes and responds to requests from the browser such as editing a page.
Describe in your own words what the View is doing in Rails View: handles presentation and it is what the user sees and interacts with. The view is not concerned about how the data is processed. The view receives results from the controller that determines what html/css will get returned to the browser.
What is the purpose of using strong params? (i.e. the params method in the controller)
How are Rails migrations related to Rails models? Rails migrations is required for any changes to be made to the table, db, datatypes in the model.
Describe one area of Rails that are still unclear on Rails is still a new concept and I need more practice.

@nkiruka nkiruka changed the title Pipes Nkiru TaskListonm Pipes Nkiru TaskList Oct 1, 2017
@Hamled
Copy link

Hamled commented Oct 6, 2017

Task List

What We're Looking For

Feature Feedback
Baseline
Appropriate Git Usage with no extraneous files checked in Mostly. Good commit practices, but there are a few files checked in that we should ignore (log/development.log and db/development.sqlite3).

We can ignore these and related files by adding the following lines to your .gitignore file:
/log/
/db/*.sqlite3
Answered comprehension questions
Successfully handles: Index, Show
Successfully handles: New, Create
Successfully handles: Edit, Update
Successfully handles: Destroy, Task Complete
Routes follow RESTful conventions
Uses named routes (like _path) ✅ There are a few extra names, but otherwise they names are correct.
Overall Overall this is good! I think more attention could be paid to judicious use of partials, and correct naming of the partial files so that the name properly reflects the contents of the file.

@@ -0,0 +1,40 @@
Rails.application.routes.draw do
get 'tasks/index'
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we need this route definition, as we already have the tasks#index route defined on line 11.

Rails.application.routes.draw do
get 'tasks/index'

get '/', to: "tasks#index", as: "home"
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could also define this route using the root directive and then use root_path instead of home_path throughout the rest of the codebase:

root "tasks#index"

# For details on the DSL available within this file, see http://guides.rubyonrails.org/routing.html

get '/tasks/new', to: "tasks#new", as: 'new_task'
post "tasks", to: "tasks#create", as: "create_task"
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This route does not need its own name, because the name for the tasks#index route (tasks) would work instead.

This is because the path portion of both routes is exactly the same (/tasks, the / at the beginning is optional).

The same applies for the route names update_task and delete_task: both of them have the exact same path (/tasks/:id) as the tasks#show route, so we can just re-use it's route name, task.

<h1 class="title">TASKLIST</h1>
<ul>
<li class="task" ><%= link_to "Add Task", new_task_path %></li>
<li class="task" ><%= link_to "All Tasks", tasks_index_path %></li>
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we remove the route on line 2 in config/routes.rb then this route helper would need to be tasks_path instead of tasks_index_path, which is more in keeping with Rails conventions.

<!-- <ol> -->
<% @tasks.each_with_index do |task, i| %>
<section>
<h3><%= "#{i + 1}. " %><%= link_to task.title, task_path(task.id), { :class => (task.complete) ? "completed" : ""} %></h3>
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice use of the ternary operator to minimize repetition of this link_to code!

<section>
<h3><%= "#{i + 1}. " %><%= link_to task.title, task_path(task.id), { :class => (task.complete) ? "completed" : ""} %></h3>

<% if task.complete == true %>
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The == true is redundant here because if statements are automatically checking for truthiness, and booleans such as task.complete have "truthiness" that matches their value of true/false.

<p><%= link_to "Mark Complete", task_path(task.id, {:task => {:complete => true, :completed_date => DateTime.now}, :refresh => true}), method: :patch %></p>
<% end %>

<p><%= link_to "Edit", task_path(task.id) %></p>
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In order for this link to go to the edit form for a given task, we would need to use the edit_task_path route helper method instead of the task_path helper, which would take us to the show page.

@@ -0,0 +1 @@
<%= render partial: "form"%>
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because the _form.html.erb partial is only used in this one location, we should just have the code from that file in this spot. Partials can be useful for helping us DRY up our view code, but only if we are actually using them in multiple places.

Additionally, "form" is probably not the ideal name for this partial as it does not contain a form but instead actually lists all of the tasks. The partial we have called "task_summary" is probably the one that should be named "form".

@@ -0,0 +1,15 @@
<% button_text ||= "The button" %>
<% form_class ||= "" %>
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like we're not actually using the button_text and form_class variables in the rest of this file. One example of how we could use those variables is:

<%= form_for @task, html: { class: form_class } do |f| %>
  <!-- the various form labels and inputs here -->
  <%= f.submit button_text %>
<% end %>

@@ -0,0 +1,53 @@
<h2>Editing Task</h2>

<%= render partial: "task_summary", locals: { submit_text: "Edit"}%>
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't use a local variable called submit_text inside of the _task_summary.html.erb partial file. I think this should probably be button_text instead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants