Update for YUI 3.6.0 #222

Closed
wants to merge 19 commits into
from

Projects

None yet

4 participants

Contributor
clarle commented Jul 11, 2012

Sorry for the huge delay. Notes for these changes, compared to the original /yuilibrary version.

  • Updated to the standardized Todos template
  • Modularized Models/Views heavily, similar to how Backbone has it, but using the YUI Loader
  • Uses Y.App for the overarching application view now
  • Supports routing via Y.App
  • Uses YUI 3.6.0 PR2
  • Uses ModelSync.Local, which I've included as a custom module since it's not on CDN yet

Ping to let @ericf know, and let me know if there's anything else I can do.

Fixes #211

Contributor
clarle commented Jul 11, 2012

Oh, right. The Todo App itself can be found here:

http://clarle.github.com/todomvc/architecture-examples/yui-3.6.0/index.html

Owner

Thank you so much for getting this updated :) We'll review the pull and let you know if it needs any more changes asap.

Owner

Very nice! Thanks for doing this on short notice :)

  • Instead of creating a new folder, can you replace the previous one?

Use a constant instead of the keyCode directly: var ENTER_KEY = 13;

  • The active todo counter should say: 1 item left where item is pluralized when it should.
  • Editing:

Make sure to .trim() the input and then check that it's not empty. If it's empty the todo should instead be destroyed.

Owner

Missing some stuff from routing. If you check a todo when e.g. on active, it should disappear right away. Filters should also be persisted on reload:

When an item is updated while in a filtered state, it should be updated accordingly. E.g. if the filter is Active and the item is checked, it should be hidden. Make sure the active filter is persisted on reload.

Contributor
clarle commented Jul 11, 2012

Ah, my bad, I actually missed the specifications entirely and was simulating it from the ones on gh-pages.

I'll get those in asap, but the Backbone one seems to not follow the 'update on filtered state' rule, and I was using that as a model. Thanks though!

Owner

Cool :)

Ya, that slipped through the Backbone app, I've created an issue for it.

@ericf ericf commented on an outdated diff Jul 11, 2012
architecture-examples/yui-3.6.0/index.html
+ 'mvc-todo-view': {
+ path: 'views/todoview.js',
+ requires: ['view', 'handlebars']
+ },
+ 'mvc-todo-app': {
+ path: 'app.js',
+ requires: ['app', 'mvc-todos', 'mvc-app-view']
+ },
+ 'model-sync-local': {
+ path: 'lib/model-sync-local.js',
+ requires: ['model', 'io-base', 'json-stringify']
+ }
+ }
+ }
+ }
+ }).use('mvc-todo-app', function (Y) {
ericf
ericf Jul 11, 2012

I think prefixing the modules with "mvc-" and having the Y.MVC namespace is confusing. How about this for the module names:

  • todo provides Y.Todo
  • todo-list provides Y.TodoList
  • todo-view provides Y.TodoView
  • todo-app provides Y.TodoApp
@ericf ericf commented on an outdated diff Jul 11, 2012
architecture-examples/yui-3.6.0/js/app.js
+ '#new-todo': {
+ keypress: 'enterCreate'
+ },
+ '#clear-completed': {
+ click: 'clearCompleted'
+ },
+ '#toggle-all': {
+ click: 'completeAll'
+ }
+ },
+
+ // Initialize our TodoList, and bind any events that occur
+ // when new Todos are added, changed, or removed within it.
+ // Also, fetch any Todos that are found within localStorage.
+ initializer: function () {
+ var list = this.todoList = new TodoList();
ericf
ericf Jul 11, 2012

This could become an Attribute of the app (that's make it more YUI-like).

@ericf ericf commented on an outdated diff Jul 11, 2012
architecture-examples/yui-3.6.0/js/models/todolist.js
+ // Dependencies from Y.MVC.
+ var Todo = Y.MVC.Todo,
+ TodoList;
+
+ // -- TodoList Model list -----
+ TodoList = Y.Base.create('todoList', Y.ModelList, [Y.ModelSync.Local], {
+
+ // The related Model for our Model List.
+ model: Todo,
+
+ // The root used for our localStorage key.
+ root: 'todos-yui',
+
+ // Return an Array of our completed Models.
+ completed: function () {
+ return this.filter(function (todo) {
ericf
ericf Jul 11, 2012

Might be useful to pass the {asList: true} option so you get a ModelList instance back from this.

@ericf ericf commented on an outdated diff Jul 11, 2012
architecture-examples/yui-3.6.0/js/models/todolist.js
+ // The related Model for our Model List.
+ model: Todo,
+
+ // The root used for our localStorage key.
+ root: 'todos-yui',
+
+ // Return an Array of our completed Models.
+ completed: function () {
+ return this.filter(function (todo) {
+ return todo.get('completed');
+ });
+ },
+
+ // Return an Array of our un-completed Models.
+ remaining: function () {
+ return this.filter(function (todo) {
ericf
ericf Jul 11, 2012

Same for this: {asList: true}

Owner

Can you also fix merge conflicts?

Clarence Leung added some commits Jul 12, 2012
Owner

@clarle Let me know when you've addressed the above stuff, and I'll do a final review.

Owner

?

Owner

@sindresorhus I think just YUI

Owner

@addyosmani Ya, I know, was more directed to the comment above.

Owner

@clarle Is this ready?

Contributor
clarle commented Jul 20, 2012

Yeah, this should be all set now. Check it out here:

http://clarle.github.com/todomvc/architecture-examples/yuilibrary/index.html

If there's any problems, just let me know. :)

Contributor
clarle commented Jul 20, 2012

And either YUI or YUI Library is fine.

Owner

Thanks :)

Found one thing:

In editing mode, removing the contents and hitting enter results in an error in Chrome: Uncaught Error: NOT_FOUND_ERR: DOM Exception 8

We've had this problem a couple of times before: #120 (comment)

Contributor
clarle commented Jul 21, 2012

Should be fixed as of the last commit. I've also re-updated my gh-pages branch with the fix so you can check it out there.

Owner

@clarle Landed. Thanks for your awesome work on this. We really appreciate you doing this last minute.

Also thanks to @ericf for reviewing :)

@gustaff-weldon gustaff-weldon pushed a commit to gustaff-weldon/todomvc that referenced this pull request Dec 23, 2013
@sindresorhus Clarence Leung + sindresorhus Close GH-222: Update for YUI 3.6.0. Fixes #211 eef4ffe
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment