Knockout.js updates for the new template and changes based on the Idiomatic style guide #132

Closed
wants to merge 5 commits into
from

Conversation

Projects
None yet
3 participants
Contributor

rniemeyer commented Apr 13, 2012

Here are the updates to the Knockout.js app to conform to the latest template. I also made changes based on the Idiomatic JS style guide and converted from spaces to tabs (in a separate commit, so diffs of the first commit are useful).

@sindresorhus - nice work on the template!

Fixes #68

-<!-- our app code -->
-<script src="js/app.js" type="text/javascript"></script>
+ <!-- Knockout has no direct dependencies -->
+ <script src="js/libs/knockout-2.0.0.js" type="text/javascript"></script>
@sindresorhus

sindresorhus Apr 13, 2012

Owner

<script src="js/libs/knockout-2.0.0.js" type="text/javascript"></script>
->
<script src="js/libs/knockout-2.0.0.js"></script>

Also use /lib instead of /libs

+ <!-- Knockout has no direct dependencies -->
+ <script src="js/libs/knockout-2.0.0.js" type="text/javascript"></script>
+ <!-- needed to support JSON.stringify in older browsers (for local storage) -->
+ <script src="js/libs/json2.js" type="text/javascript"></script>
@sindresorhus

sindresorhus Apr 13, 2012

Owner

Not needed. We only support IE8+, which does support JSON.

+ <header id="header">
+ <h1>todos</h1>
+ <input id="new-todo" type="text" data-bind="value: current, valueUpdate: 'afterkeydown', enterKey: add"
+ placeholder="What needs to be done?" autofocus />
@sindresorhus

sindresorhus Apr 13, 2012

Owner

autofocus /> -> autofocus>

+
+ //wrap the handler with a check for the enter key
+ wrappedHandler = function( data, event ) {
+ if ( event.keyCode === 13 ) {
@sindresorhus

sindresorhus Apr 13, 2012

Owner

Use constant.

var ENTER_KEY = 13;

...

if ( event.keyCode === ENTER_KEY ) {
+ };
+
+ //alternative to "visible" binding that will specifically set "block" to override what is in css
+ ko.bindingHandlers.block = {
@sindresorhus

sindresorhus Apr 13, 2012

Owner

This looks a little dirty. Any better way to do it?

You know you can override the base.css in app.css if needed.

@addyosmani

addyosmani Apr 13, 2012

Owner

+1 if we can avoid overriding this in JS that would be great. It also allows us the flexibility to make changes to the template in the future without worrying about anything other than markup and CSS.

@rniemeyer

rniemeyer Apr 13, 2012

Contributor

I'll put in the app.css, now that it is standard. Will allow me to remove the two custom bindings and use the standard visible binding. Will be no problem.

@rniemeyer

rniemeyer Apr 14, 2012

Contributor

I hadn't studied the latest base.css closely enough. I was able to use the standard "visible" binding now without using an override css file, since a few display: none styles were removed.

+ });
+ };
+
+ //edit an item
@sindresorhus

sindresorhus Apr 13, 2012

Owner

Nitpick, but //edit an item -> // edit an item

+ <footer id="footer" data-bind="block: completedCount() || remainingCount()">
+ <span id="todo-count">
+ <strong data-bind="text: remainingCount">1</strong>
+ <span data-bind="text: getLabel( remainingCount )"></span> left.
@sindresorhus

sindresorhus Apr 13, 2012

Owner

Remove the end dot

Owner

sindresorhus commented Apr 13, 2012

Awesome work!

I've added some inline comments.

Contributor

rniemeyer commented Apr 14, 2012

@sindresorhus here are all of the additional changes including "complete" to "completed"

Owner

sindresorhus commented Apr 14, 2012

Looks good, thanks :)

Could you remove the Amplify dependency? It doesn't add anything since the browsers we support have localStorage.

Can you use completed in the model instead of done? Consistency.

+ };
+
+ // check local storage for todos
+ var todos = amplify.store( 'todos-knockout' );
@sindresorhus

sindresorhus Apr 14, 2012

Owner

Something like:

var store = localStorage.getItem('todos-knockout');
var todos = ( store && JSON.parse( store ) ) || [];
+ var todos = ko.toJS( self.todos );
+
+ // store to local storage
+ amplify.store( 'todos-knockout', todos );
@sindresorhus

sindresorhus Apr 14, 2012

Owner
localStorage.setItem( 'todos-knockout', JSON.stringify( data ) );
+ var Todo = function( content, done ) {
+ this.content = ko.observable( content );
+ this.done = ko.observable( done );
+ this.editing = ko.observable( false );
@sindresorhus

sindresorhus Apr 14, 2012

Owner

content -> title
done -> completed

var Todo = function( title, completed ) {
this.title = ko.observable( title );
this.completed = ko.observable( completed );
this.editing = ko.observable( false );

This also applies other places in the code.

Contributor

rniemeyer commented Apr 14, 2012

Made additional updates.

+ // stop editing an item. Remove the item, if it is now empty
+ self.stopEditing = function( item ) {
+ item.editing( false );
+ if ( !item.content().trim() ) {
@sindresorhus

sindresorhus Apr 14, 2012

Owner

if ( !item.content().trim() ) { -> if ( !item.title().trim() ) {

Contributor

rniemeyer commented Apr 14, 2012

Not sure how I missed that one. Thanks.

Owner

sindresorhus commented Apr 14, 2012

Landed in commit 9120533.

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

Landing pull request 132. Knockout.js updates for the new template an…
…d changes based on the Idiomatic style guide Fixes #????.

More Details:
 - tastejs#132
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment