todo application made with Agilityjs. #129

Closed
wants to merge 17 commits into
from

Projects

None yet

2 participants

Contributor
tshm commented Apr 12, 2012

This is a retry of the previous pull-request #115.
I hope it now comply with all the requirements.

  • Now it is in a separate branch, not in a master branch.
    I hope this is preferable.
  • Library is AgilityJS: http://agilityjs.com/
  • localStorage is not directly supported by AgilityJS, so it is
    done via my custom adapter for agilityjs storage mechanism.

Thank you.

Tosh Shimayma

Owner

Awesome, again thanks for this :)

I've thoroughly reviewed the implementation and have some feedback:

  • Remove the default Create a TodoMVC template todo HTML, it's only there to show the structure
  • Clear completed (1 item) should be Clear completed (1)
  • When in editing mode, it doesn't save on Enter or Blur.
  • x item left: this should be the count of the active todos (not marked as done), not the count of all the todos.
  • Make sure to follow the Idiomatic guide to writing JavaScript. Especially the part about whitespace inside parens, not dropping braces, and linebreak after opening braces.
  • Remove console.log

We decided to make the project Public Domain. Would you concider removing the MIT license?

@sindresorhus sindresorhus commented on an outdated diff Apr 12, 2012
architecture-examples/agilityjs/index.html
@@ -0,0 +1,66 @@
+<!doctype html>
+<html lang="en">
+<head>
+ <meta charset="utf-8">
+ <title>agilityjs • TodoMVC</title>
sindresorhus
sindresorhus Apr 12, 2012 Owner

Agility.js • TodoMVC

@sindresorhus sindresorhus commented on an outdated diff Apr 12, 2012
architecture-examples/agilityjs/index.html
+ <meta charset="utf-8">
+ <title>agilityjs • TodoMVC</title>
+ <link rel="stylesheet" href="../../assets/base.css">
+</head>
+<body>
+ <section id="todoapp">
+ <header id="header">
+ <h1>todos</h1>
+ <input id="new-todo" type="text" data-bind="newtitle" placeholder="What needs to be done?" autofocus>
+ </header>
+
+ <section id="main" data-bind="class = mainStyle">
+ <input id="toggle-all" type="checkbox" data-bind="toggleAll">
+ <label for="toggle-all">Mark all as complete</label>
+ <ul id="todo-list">
+ <li class="complete">
sindresorhus
sindresorhus Apr 12, 2012 Owner

Ref the review, remove this list item.

@sindresorhus sindresorhus commented on an outdated diff Apr 12, 2012
architecture-examples/agilityjs/index.html
+ <section id="main" data-bind="class = mainStyle">
+ <input id="toggle-all" type="checkbox" data-bind="toggleAll">
+ <label for="toggle-all">Mark all as complete</label>
+ <ul id="todo-list">
+ <li class="complete">
+ <div class="view">
+ <input class="toggle" type="checkbox" checked>
+ <label>Create a TodoMVC template</label>
+ <button class="destroy"></button>
+ </div>
+ <input class="edit" type="text" value="Create a TodoMVC template">
+ </li>
+ <li data-bind="class = status">
+ <div class="view">
+ <input class="toggle" type="checkbox" data-bind="complete">
+ <label data-bind="title">Rule the web</label>
sindresorhus
sindresorhus Apr 12, 2012 Owner

Remove the tag contents - Rule the web

@sindresorhus sindresorhus commented on an outdated diff Apr 12, 2012
architecture-examples/agilityjs/index.html
+ <ul id="todo-list">
+ <li class="complete">
+ <div class="view">
+ <input class="toggle" type="checkbox" checked>
+ <label>Create a TodoMVC template</label>
+ <button class="destroy"></button>
+ </div>
+ <input class="edit" type="text" value="Create a TodoMVC template">
+ </li>
+ <li data-bind="class = status">
+ <div class="view">
+ <input class="toggle" type="checkbox" data-bind="complete">
+ <label data-bind="title">Rule the web</label>
+ <button class="destroy"></button>
+ </div>
+ <input class="edit" type="text" value="Rule the web" data-bind="title">
sindresorhus
sindresorhus Apr 12, 2012 Owner

Remove the value attribute

@sindresorhus sindresorhus commented on an outdated diff Apr 12, 2012
architecture-examples/agilityjs/js/app.js
@@ -0,0 +1,166 @@
+/*
+
+[MIT licensed](http://en.wikipedia.org/wiki/MIT_License)
+(c) [Toshihide Shimayama](http://github.com/tshm/todomvc/)
+
+*/
+(function($, $$, console) {
+ 'use strict';
+ // Hack of taking out html elements from DOM so that agility's view can use it.
+ var drawHtml = function(selector) {
+ // we need 'outerhtml' also, as agilityjs will append DOM, so removing it.
+ var html = $(selector).remove().wrap('<div>').parent().html();
+ if (!html) {
sindresorhus
sindresorhus Apr 12, 2012 Owner

Per review: Add space inside parens.

Eg:
if ( !html ) {

@sindresorhus sindresorhus commented on an outdated diff Apr 12, 2012
architecture-examples/agilityjs/js/app.js
+ var html = $(selector).remove().wrap('<div>').parent().html();
+ if (!html) {
+ console.log('failed extracting: ' + selector);
+ }
+ return html;
+ };
+
+ // Simple Two layer composition:
+ // individual 'todoitem' and 'app'lication which holds multiple todoitems.
+ $(function() {
+ // remove example item already given in the template
+ var $sampleTodo = $('#todo-list li.complete').remove();
+
+ // todo item
+ var todoitem = $$({
+ model: { complete: false },
sindresorhus
sindresorhus Apr 12, 2012 Owner

Per review about linebreak after opening brace. Like other style changes, this applies in multiple places.

model: { complete: false },

->

model: {
    complete: false
},
@sindresorhus sindresorhus commented on an outdated diff Apr 12, 2012
architecture-examples/agilityjs/js/app.js
+ }
+ return html;
+ };
+
+ // Simple Two layer composition:
+ // individual 'todoitem' and 'app'lication which holds multiple todoitems.
+ $(function() {
+ // remove example item already given in the template
+ var $sampleTodo = $('#todo-list li.complete').remove();
+
+ // todo item
+ var todoitem = $$({
+ model: { complete: false },
+ view: {
+ format: drawHtml('#todo-list li'),
+ style: '.hidden {display: none;}'
sindresorhus
sindresorhus Apr 12, 2012 Owner

style: '.hidden {display: none;}' -> style: '.hidden { display: none }'

@sindresorhus sindresorhus commented on an outdated diff Apr 12, 2012
architecture-examples/agilityjs/js/app.js
+ if (title) {
+ this.model.set({title: title});
+ } else {
+ this.destroy();
+ }
+ }
+ }).persist($$.adapter.localStorage, {collection: 'todos-agilityjs'});
+
+ // The main application which holds todo items.
+ var app = $$({
+ model: {
+ count: '0',
+ pluralizer: '',
+ completeCountText: '0 item',
+ newtitle: '',
+ toggleAll:false,
sindresorhus
sindresorhus Apr 12, 2012 Owner

Space after :

toggleAll:false, -> toggleAll: false,

@sindresorhus sindresorhus commented on an outdated diff Apr 12, 2012
architecture-examples/agilityjs/js/app.js
+ count: '0',
+ pluralizer: '',
+ completeCountText: '0 item',
+ newtitle: '',
+ toggleAll:false,
+ mainStyle:'',
+ clearBtnStyle:''
+ },
+ view: {
+ format: drawHtml('#todoapp'),
+ style: '.hidden {display: none;}'
+ },
+ controller: {
+ 'remove': function() { app.updateStatus(); },
+ 'keyup #new-todo': function(event) {
+ if (13 !== event.which) { return; }
sindresorhus
sindresorhus Apr 12, 2012 Owner

Reverse the comparison and add the 13 as a constant.

if (13 !== event.which) { return; } ->

var ENTER_KEY = 13;

...

if ( event.which !== ENTER_KEY ) {
    return; 
}
@sindresorhus sindresorhus commented on an outdated diff Apr 12, 2012
architecture-examples/agilityjs/js/app.js
+ completeCountText: '0 item',
+ newtitle: '',
+ toggleAll:false,
+ mainStyle:'',
+ clearBtnStyle:''
+ },
+ view: {
+ format: drawHtml('#todoapp'),
+ style: '.hidden {display: none;}'
+ },
+ controller: {
+ 'remove': function() { app.updateStatus(); },
+ 'keyup #new-todo': function(event) {
+ if (13 !== event.which) { return; }
+ var title = this.model.get('newtitle').trim();
+ if (!title) return;
sindresorhus
sindresorhus Apr 12, 2012 Owner

Use braces.

if (!title) return; ->

if ( !title ) {
    return;
}
@sindresorhus sindresorhus commented on an outdated diff Apr 12, 2012
architecture-examples/agilityjs/js/localstorage.js
+/*
+
+ [MIT licensed](http://en.wikipedia.org/wiki/MIT_License)
+ (c) [Toshihide Shimayama](http://github.com/tshm/todomvc/)
+
+*/
+// custom agilityjs adapter for localstorage
+(function($$, console) {
+ 'use strict';
+
+ $$.adapter.localStorage = function(_params) {
+ var storageKey = (this._data.persist.baseUrl || '') + this._data.persist.collection,
+ storageStr = localStorage[storageKey],
+ items = (storageStr ? JSON.parse(storageStr) : {});
+ //
+ if ('GET' === _params.type) {
sindresorhus
sindresorhus Apr 12, 2012 Owner

Reverse comparison

Contributor
tshm commented Apr 13, 2012

Thank you for the detailed review.
I think I addressed everything now.

  • better coding standard compliance.
  • removal of MIT license.
  • todoCount misunderstanding fix.
  • typo bug fixes.
  • cleanup sample todo items from the template html.

Thank you again for the opportunity.

tosh shimayama comparison order fix ca43920
@sindresorhus sindresorhus commented on an outdated diff Apr 13, 2012
architecture-examples/agilityjs/index.html
+ <a href="#/active">Active</a>
+ </li>
+ <li>
+ <a href="#/completed">Completed</a>
+ </li>
+ </ul>
+ <button id="clear-completed" data-bind="class = clearBtnStyle">Clear completed (<span data-bind="completeCount"></span>)</button>
+ </footer>
+ </section>
+ <footer id="info">
+ <p>Double-click to edit a todo</p>
+ <p>Template by <a href="http://sindresorhus.com">Sindre Sorhus</a></p>
+ <p>Created by <a href="http://github.com/tshm/todomvc/">Tosh Shimayama</a></p>
+ </footer>
+
+ <script src="http://ajax.googleapis.com/ajax/libs/jquery/1.7.1/jquery.min.js"></script>
sindresorhus
sindresorhus Apr 13, 2012 Owner

Use the jQuery in the assets folder instead

@sindresorhus sindresorhus commented on an outdated diff Apr 13, 2012
architecture-examples/agilityjs/js/app.js
@@ -0,0 +1,179 @@
+(function($, $$) {
sindresorhus
sindresorhus Apr 13, 2012 Owner

(function($, $$) { -> (function( $, $$ ) {

Applies in multiple places.

@sindresorhus sindresorhus commented on an outdated diff Apr 13, 2012
architecture-examples/agilityjs/js/app.js
@@ -0,0 +1,179 @@
+(function($, $$) {
+ 'use strict';
+ var ENTER_KEY = 13;
+
+ // Hack of taking out html elements from DOM so that agility's view can use it.
+ var drawHtml = function(selector) {
sindresorhus
sindresorhus Apr 13, 2012 Owner

Space inside parens. Applies multiple places.

function(selector) { -> function( selector ) {

@sindresorhus sindresorhus commented on an outdated diff Apr 13, 2012
architecture-examples/agilityjs/js/app.js
+ }
+ },
+ // utility functions
+ setStatus: function(status) {
+ this.model.set({status: status});
+ },
+ updateTitle: function() {
+ this.setStatus('');
+ var title = this.model.get('title').trim();
+ if ( title ) {
+ this.model.set({title: title});
+ } else {
+ this.destroy();
+ }
+ }
+ }).persist($$.adapter.localStorage, {collection: 'todos-agilityjs'});
sindresorhus
sindresorhus Apr 13, 2012 Owner

{collection: 'todos-agilityjs'});

->

{
    collection: 'todos-agilityjs'
});
Owner

Thanks for doing this and sorry for the nitpick. I've added a few more inline comments. See above.

tosh shimayama added some commits Apr 13, 2012
Contributor
tshm commented Apr 13, 2012

some questions regarding putting spaces inside the function call parens
especially when the argument takes anonymous objects or functions arguments.

Should I do following changes as well?

  • jQuery shorthand selector calls: ie. $('#id') -> $( '#id' )
  • anonymouse function as an argument. (this is multi-line.) : $.each(function() { ... }) -> $.each( function() {...} )
  • Object as an function all argument (this is multiline as well):
    func({
    key: value
    })
    ->
    func( {
    key: value
    } )
  • mixture of 2 and 3. (anonymous function part becomes multi-line) e.g.
    $element.on('click', function() { ... })
    ->
    $element.on( 'click', function() { ... } )

Thanks

tosh shimayama add spaces in function def & call
- one space after '(' and before ')' if
  the argument is not anonymous object/function.
- except jquery shorthand calls.
f714fab
Contributor
tshm commented Apr 13, 2012

I think I got the idea.
The case: 'anonymous object/function being the function argument' is discussed
in the exception & deviation sub-section in the whitespace rule section.

I hope it is better now.

Thanks

tosh shimayama remove view only properties
In Agility.js, there is no separation between
model and view-model.  Properties are all mixed up.

I tried to remove as many view only properties as possible
from the todoitem so that it'll not persist them.
a5ddcaf

title: 'no name', -> title: '',

Use completed instead of complete, my fault... forgot the d in an earlier version of the spec. Make sure you update everywhere you use complete. Sorry about this.

Never assign in a statement.

->

var title = $('#new-todo').val().trim();
if ( event.which === ENTER_KEY && title ) {

I know you did it so not to get the val() when not enter, but the performance benefit is negligent.

-> var item = $$( todoitem, {

-> item.view.$().toggleClass( 'hidden', !isVisible( item ) );

-> this.view.$().removeClass('editing');

Owner

Thanks :)

I've added some more inline comments.


jQuery shorthand selector calls: ie. $('#id') -> $( '#id' )

No, there's an exception on single arguments that are quoted, like this.

anonymouse function as an argument. (this is multi-line.) : $.each(function() { ... }) -> $.each( function() {...} )

No, this is also an exception

Object as an function all argument (this is multiline as well): func({ key: value }) -> func( { key: value } )

No, same as above

mixture of 2 and 3. (anonymous function part becomes multi-line) e.g. $element.on('click', function() { ... }) -> $element.on( 'click', function() { ... } )

Yes

Owner

Let me know when you've done the required changes and I'll review it one last time before pulling it in.

Contributor
tshm commented Apr 19, 2012

Please review it.
Thanks.

@gustaff-weldon gustaff-weldon pushed a commit to gustaff-weldon/todomvc that referenced this pull request Dec 23, 2013
@tshm @sindresorhus tshm + sindresorhus Closes #129: todo application made with Agilityjs.. Fixes #115 328bbaf
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment