Agilityjs pull request for fixing issue #257 #275

Closed
wants to merge 9 commits into
from

Projects

None yet

3 participants

@tshm
Contributor
tshm commented Aug 27, 2012

This change set should fix issue #257.

Thanks

Fixes #257

@sindresorhus sindresorhus commented on an outdated diff Aug 27, 2012
architecture-examples/agilityjs/js/app.js
@@ -117,13 +125,15 @@
});
this.model.set({
todoCount: count - completedCount + '',
- pluralizer: (count > 1 ? 's' : ''),
+ pluralizer: (count - completedCount > 1 ? 's' : ''),
@sindresorhus
sindresorhus Aug 27, 2012 Member

pluralizer: count - completedCount === 1 ? '' : 's',

@sindresorhus
Member
  • Getting error in console on each action I do:
4Uncaught TypeError: undefined is not a function app.js:153
15
Uncaught TypeError: undefined is not a function 
  • When entering a new todo and pressing enter, the todo content remains even though the todo is added.
@addyosmani
Member

Just reviewed. You're close :)

The only issue remaining that I see is when you edit an existing item and blur out, there are errors logged to the console. http://cl.ly/image/3B0t0K0P1F0H. If you can file a fix for this, I'll merge.

Thanks!

Edit: and the pluralizer issue mentioned below.

@sindresorhus
Member

The pluralizer is still not correct, please follow my comment: pluralizer: count - completedCount === 1 ? '' : 's',.

@tshm
Contributor
tshm commented Sep 2, 2012

Sorry about the defects.
I addressed the pluralizer issue, but cannot reproduce the console logs.
I tested with google chrome 21.0.1180.89 and Firefox 15.0 (with FireBug) with
opening index.html via file protocol and http served by node.js web server.

Could you describe how I can reproduce the console issue?

Thank you very much for your help.

@tshm tshm closed this Sep 2, 2012
@tshm tshm reopened this Sep 2, 2012
@sindresorhus sindresorhus added a commit that closed this pull request Sep 3, 2012
@sindresorhus tosh shimayama + sindresorhus Closes #275: Update Agility.js app. Fixes #257 a52a4fc
@sindresorhus
Member

@tshm Landed. Thanks :)

@addyosmani I couldn't reproduce either. Both Chrome 21 and Canary. Might be a temp issue or an extension or something.

@gustaff-weldon gustaff-weldon pushed a commit to gustaff-weldon/todomvc that referenced this pull request Dec 23, 2013
@sindresorhus tosh shimayama + sindresorhus Closes #275: Update Agility.js app. Fixes #257 cbbce90
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment