Skip to content
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

css #18

Open
wants to merge 23 commits into
base: master
Choose a base branch
from
Open

css #18

wants to merge 23 commits into from

Conversation

pottery123
Copy link


def delete

@tasklist = RailsTaskList.find(params[:id])

Choose a reason for hiding this comment

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

You repeat this line in a few places, so you could possibly it out into a before_action only for certain controller methods if you don't think that would reduce clarity.

@daphnegold
Copy link

daphnegold commented Apr 30, 2016

Nice! What you have here is really well-thought-out.

Some things to think about:

  • _Indentation & whitespace_
    It not only makes your code more readable and organized, but some languages actually rely on whitespace to run! So it's good practice.
  • _Meaningful commit messages_
    Your commit messages don't need to be long, but a brief description of what you changed really helps code reviewers see your progression. If problems come up it can also help you pinpoint where those might have occurred.
  • _Pesky .ds_store files belong in your .gitignore_
    Apple computers add .ds_store files to every directory you visit with the finder. These are just taking up space when committed to Git. I am the queen of committing files erroneously, and it can be really tedious to remove them from your history later!

data: { confirm: 'Are you sure?' } %>
<span>

<span><%=link_to "Done!",done_task_list_path(task.id),method: :patch%></span>
Copy link

@daphnegold daphnegold Apr 30, 2016

Choose a reason for hiding this comment

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

This link doesn't appear to be working.

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