Updating Angular demo to v1.0 of the library #157

Closed
wants to merge 3 commits into
from

Conversation

Projects
None yet
4 participants
@ebidel
Contributor

ebidel commented May 3, 2012

No description provided.

@addyosmani

This comment has been minimized.

Show comment Hide comment
@addyosmani

addyosmani May 3, 2012

Owner

Thanks for the PR, Eric! We'll review this asap and let you know if it's good for a merge :)

Owner

addyosmani commented May 3, 2012

Thanks for the PR, Eric! We'll review this asap and let you know if it's good for a merge :)

@sindresorhus

This comment has been minimized.

Show comment Hide comment
@sindresorhus

sindresorhus May 3, 2012

Owner

@cburgdorf Do mind checking this out?

Tip: Append ?w=1 to the url to ignore whitespace changes. Makes it a lot more clearer.

Owner

sindresorhus commented May 3, 2012

@cburgdorf Do mind checking this out?

Tip: Append ?w=1 to the url to ignore whitespace changes. Makes it a lot more clearer.

@cburgdorf

This comment has been minimized.

Show comment Hide comment
@cburgdorf

cburgdorf May 3, 2012

Contributor

Hey @ebidel , thanks for jumping on it! I made some comments. Otherwise, looks good to me ;-)

Contributor

cburgdorf commented May 3, 2012

Hey @ebidel , thanks for jumping on it! I made some comments. Otherwise, looks good to me ;-)

@cburgdorf

This comment has been minimized.

Show comment Hide comment
@cburgdorf

cburgdorf May 3, 2012

Contributor

@sindresorhus thanks for the ?w=1 hint. Did not know about that so far!

Contributor

cburgdorf commented May 3, 2012

@sindresorhus thanks for the ?w=1 hint. Did not know about that so far!

@sindresorhus

This comment has been minimized.

Show comment Hide comment
@sindresorhus

sindresorhus May 3, 2012

Owner

@cburgdorf Yes, it's immensely useful on big diffs with a lot of whitespace changes. Too bad it's hidden behind a url parameter...

Owner

sindresorhus commented May 3, 2012

@cburgdorf Yes, it's immensely useful on big diffs with a lot of whitespace changes. Too bad it's hidden behind a url parameter...

@ebidel

This comment has been minimized.

Show comment Hide comment
@ebidel

ebidel May 3, 2012

Contributor

@cburgdorf Addressed your comments. Thanks for those.

Apparently $watch has changed a bit since the original demo. I finally figured it out and we're using that again.

Contributor

ebidel commented May 3, 2012

@cburgdorf Addressed your comments. Thanks for those.

Apparently $watch has changed a bit since the original demo. I finally figured it out and we're using that again.

@cburgdorf

This comment has been minimized.

Show comment Hide comment
@cburgdorf

cburgdorf May 3, 2012

Contributor

Great! You know, the reason why I used $watch in the first place was that I otherwise wasn't able to update the store when one of the checkboxes changed. According to the spec, the store should also be updated when the checkbox on the todo changes its state. However, I just figured out that you can use ng-change for checkboxes to catch that moment. I think back in the days when I wrote that app there was no ng-change that worked for checkboxes. On the other hand, for such a small demo app, I think it's more ideomatic to just leverage $watch in the way we do it here. It's more DRY at least.

Contributor

cburgdorf commented May 3, 2012

Great! You know, the reason why I used $watch in the first place was that I otherwise wasn't able to update the store when one of the checkboxes changed. According to the spec, the store should also be updated when the checkbox on the todo changes its state. However, I just figured out that you can use ng-change for checkboxes to catch that moment. I think back in the days when I wrote that app there was no ng-change that worked for checkboxes. On the other hand, for such a small demo app, I think it's more ideomatic to just leverage $watch in the way we do it here. It's more DRY at least.

@ebidel

This comment has been minimized.

Show comment Hide comment
@ebidel

ebidel May 3, 2012

Contributor

Yep. They've added a bunch of stuff since you did the original demo. Two-way data-binding to checkboxes can come with ng-change or ng-checked.

Contributor

ebidel commented May 3, 2012

Yep. They've added a bunch of stuff since you did the original demo. Two-way data-binding to checkboxes can come with ng-change or ng-checked.

@ebidel

This comment has been minimized.

Show comment Hide comment
@ebidel

ebidel May 4, 2012

Contributor

Think this is ready for a merge :)

Contributor

ebidel commented May 4, 2012

Think this is ready for a merge :)

@sindresorhus

This comment has been minimized.

Show comment Hide comment
@sindresorhus

sindresorhus May 4, 2012

Owner

@ebidel Thanks :D

Owner

sindresorhus commented May 4, 2012

@ebidel Thanks :D

gustaff-weldon pushed a commit to gustaff-weldon/todomvc that referenced this pull request Dec 23, 2013

gustaff-weldon pushed a commit to gustaff-weldon/todomvc that referenced this pull request Dec 23, 2013

Merge branch 'master' of github.com:addyosmani/todomvc
* 'master' of github.com:addyosmani/todomvc:
  Close GH-157: Updating Angular demo to v1.0 of the library.
  various readme changes. some minor, some necessary to move towards cleaner markdown
  very minor change for consistency in comment spacing/casing
  updating readme to include links, revised team notes and other tweaks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment