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

Spine todo app rewrite #127

Closed
wants to merge 2 commits into from
Closed

Spine todo app rewrite #127

wants to merge 2 commits into from

Conversation

sindresorhus
Copy link
Member

With routing. Fixes #70.

Depends on pull #126.

@addyosmani
Copy link
Member

I just went through this and it looks good to merge. Would you like me to ask Alex MacCaw for a quick code review or are you pretty happy there are no further improvements we can probably make to it?

@sindresorhus
Copy link
Member Author

I'm pretty happy with it. Though it's the first time I use CoffeScript or Spine, so I might be doing something stupid. There are a couple of TODOs in there, which I just couldn't figure out. One of them looks like a Spine bug.

I would love a code review if he's interested in doing it 😃

…rewrite

* 'master' of github.com:addyosmani/todomvc:
  Delicious new template

render: =>
@replace TPL( @todo )
@

Choose a reason for hiding this comment

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

I know this is perfectly valid coffee code, but it isn't that readable IMHO. What about a simple "this" ?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't see how using this is anymore readable. @ is used in the rest of the code, and I want to be consistent.

Choose a reason for hiding this comment

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

I'm only talking when it's alone. It doesn't feel clear to me, that's all.

gustaff-weldon pushed a commit to gustaff-weldon/todomvc that referenced this pull request Dec 23, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update spine example
3 participants