TodoMVC using Mojito (v0.5.6 - with updates) #507

Closed
wants to merge 4 commits into
from

Projects

None yet

6 participants

@gvaish
gvaish commented Mar 28, 2013

Originally for #435 - couldn't find a way to update the same pull request - may be because I scratched the original fork rather than rebasing again.

Commit log

The files in the folder comprise of only the code and not the final running application.

After all hits-and-tries realized that I cannot make it up and running on Github because the application needs to make calls to POST /tunnel and Github pages won't allow that.

@passy
Member
passy commented Mar 28, 2013

Thanks for the update. For the next time, please force-push to your existing branch and don't open another PR, as @sindresorhus said. Having a new PR for every change gets really confusing quickly.

@gvaish
gvaish commented Mar 28, 2013

@passy I know... I also have multiple PRs - it's painful as well!

@gvaish
gvaish commented Mar 28, 2013

@caridy - Can you update your task at https://trello.com/card/review-todomvc-by-gaurav/504e5e3b7cd1578456fc430f/406 for this pull request?

@passy
Member
passy commented Apr 4, 2013

@gvaish Could you check our Contribution Guidelines and make sure to match the code style mentioned there? :)

@gvaish
gvaish commented Apr 5, 2013

@passy Yikes. Seems like some work for me this weekend as well! Well, I think I'll pick this up early next week.

@addyosmani
Member

Thanks @gvaish. We appreciate the extra time you're putting in to match our code style. Makes comparison a little more easy :)

@gvaish
gvaish commented Apr 12, 2013

Anytime, @addyosmani! :-)
Actually, browsing through I realized that I'd been missing a few test cases as well.

@gvaish gvaish @gvaish Squashing commits
@gvaish Todo using Mojito

The files in the folder comprise of only the code and not the final running application.
After all hits-and-tries realized that I cannot make it up and running on Github because the application needs to make calls to "POST /tunnel" and Github pages won't allow that.

* readme.md: Readme
* **/*: Code files

@gvaish Updating latest code from the base project at https://github.com/gvaish/todomvc-mojito/tree/master/todomvc-mojito-common
ac94dbb
@gvaish
gvaish commented Apr 13, 2013

Done.

  1. Squashed commits - did it after eons
  2. Unit Tests - did a lot of them
  3. Coding style - was mostly as mine, so not much of a trouble. jshint only complained of bad option anon, sloppy for jslint, but left in the original code as it's auto generated. It also complained of confusing use of !! at line 37, col 37 - again left it as it is valid.
@gvaish
gvaish commented Apr 16, 2013

@passy / @addyosmani - Can you look at it now?

@caridy - Can you look at from Mojito's perspective... may be there's something that you may want to suggest / change / improve.

Changes squashed at gvaish/todomvc@ac94dbb

@passy
Member
passy commented Apr 16, 2013

@gvaish I'm very happy to see tests here. :)

Could you run jshint with our config on your code? I'm still seeing a few styleguide deviations like function() instead of function ().

@addyosmani
Member

Ping @caridy for any suggestions or a 👍 :)

I really appreciate seeing tests here too, @gvaish - well done. I think this should be good for us to land once we're happy there are no longer styleguide issues.

@caridy caridy and 1 other commented on an outdated diff Apr 23, 2013
labs/architecture-examples/mojito/application.json
+ {
+ "settings": [ "master" ],
+ "specs": {
+ "main_ui": {
+ "type": "HTMLFrameMojit",
+ "config": {
+ "deploy": true,
+ "title": "Yahoo! Mojito • TodoMVC",
+ "child": { "type": "TodoMojit" }
+ }
+ }
+ },
+ "appPort": 6789,
+ "staticHandling": {
+ "appName": "todomvc",
+ "useRollups": false
@gvaish
gvaish Apr 23, 2013

The staticHandling or useRollups?

@caridy caridy and 1 other commented on an outdated diff Apr 23, 2013
labs/architecture-examples/mojito/application.json
+ "specs": {
+ "main_ui": {
+ "type": "HTMLFrameMojit",
+ "config": {
+ "deploy": true,
+ "title": "Yahoo! Mojito • TodoMVC",
+ "child": { "type": "TodoMojit" }
+ }
+ }
+ },
+ "appPort": 6789,
+ "staticHandling": {
+ "appName": "todomvc",
+ "useRollups": false
+ },
+ "log": {
@caridy
caridy Apr 23, 2013

deprecated, make sure log levels are defined under yui.config settings. Check the docs for more details.

@caridy caridy commented on an outdated diff Apr 23, 2013
labs/architecture-examples/mojito/application.json
+ "config": {
+ "combine": false
+ }
+ },
+ "builds": {
+ "html5app": {
+ "forceRelativePaths": true,
+ "attachManifest": false
+ }
+ }
+ },
+ {
+ "settings": [ "environment:development" ],
+ "staticHandling": {
+ "forceUpdate": true,
+ "useRollups": false
@caridy caridy commented on an outdated diff Apr 23, 2013
labs/architecture-examples/mojito/index.js
@@ -0,0 +1,23 @@
@caridy
caridy Apr 23, 2013

you don't really need this file if you're not deploying to manhattan.

@caridy caridy and 1 other commented on an outdated diff Apr 23, 2013
...ure-examples/mojito/mojits/TodoMojit/binders/index.js
@@ -0,0 +1,216 @@
+/*jslint anon:true, sloppy:true, nomen:true*/
+YUI.add('TodoMojitBinderIndex', function(Y, NAME) {
+ "use strict";
+
+ Y.namespace('mojito.binders')[NAME] = {
+
+ init: function(mojitProxy) {
+ this.mp = mojitProxy;
+ },
+
+ bind: function(node) {
+ var self = this;
@caridy
caridy Apr 23, 2013

not needed, since u don't have any async process in here.

@gvaish
gvaish Apr 23, 2013

he he... I had some code earlier and then cleaned up leaving this behind.
Will cleanup this as well.

@caridy caridy commented on an outdated diff Apr 23, 2013
...ure-examples/mojito/mojits/TodoMojit/binders/index.js
+ init: function(mojitProxy) {
+ this.mp = mojitProxy;
+ },
+
+ bind: function(node) {
+ var self = this;
+
+ this.node = node;
+ this.inputNode = node.one('#new-todo');
+ this.listNode = node.one('#todo-list');
+ this.toggleAll = node.one('#toggle-all');
+ this.main = node.one('#main');
+
+ this.addHandlers();
+
+ self.resync();
@caridy
caridy Apr 23, 2013

this.resync() is just fine.

@caridy caridy and 1 other commented on an outdated diff Apr 23, 2013
...ure-examples/mojito/mojits/TodoMojit/binders/index.js
+ //li[complete ? 'addClass' : 'removeClass']('completed');
+ self.resync();
+ }
+ });
+ },
+
+ startEdit: function(e) {
+ var lbl = e.currentTarget,
+ li = lbl.ancestor('li');
+
+ li.addClass('editing');
+ li.one('.edit').focus().select();
+ },
+
+ stopEdit: function(e) {
+ //alert('value: ' + e.currentTarget.get("value"));
@caridy
caridy Apr 23, 2013

remove alerts please!

@caridy caridy and 1 other commented on an outdated diff Apr 23, 2013
...examples/mojito/mojits/TodoMojit/controller.common.js
@@ -0,0 +1,140 @@
+/*jslint anon:true, sloppy:true, nomen:true*/
+YUI.add('TodoMojit', function(Y, NAME) {
+ "use strict";
+
+ Y.namespace('mojito.controllers')[NAME] = {
+
+ index: function(ac) {
+ ac.assets.addBlob('<meta charset="utf-8">\n<meta http-equiv="X-UA-Compatible" content="IE=edge,chrome=1">', 'top');
+ ac.assets.addBlob('<link href="/static/' + ac.type + '/assets/base.css" rel="stylesheet" type="text/css" />', 'top');
@caridy
caridy Apr 23, 2013

mojito can do this for u, just add the css as relative path and mojito will compute the prefix and the mojit name under the hood.

@gvaish
gvaish Apr 23, 2013

Yes. I guess, that's 0.4.x code. I always had at the back of my mind that I'd need to update it. And you caught it ;-)

@caridy caridy and 1 other commented on an outdated diff Apr 23, 2013
...examples/mojito/mojits/TodoMojit/controller.common.js
@@ -0,0 +1,140 @@
+/*jslint anon:true, sloppy:true, nomen:true*/
+YUI.add('TodoMojit', function(Y, NAME) {
+ "use strict";
+
+ Y.namespace('mojito.controllers')[NAME] = {
+
+ index: function(ac) {
+ ac.assets.addBlob('<meta charset="utf-8">\n<meta http-equiv="X-UA-Compatible" content="IE=edge,chrome=1">', 'top');
+ ac.assets.addBlob('<link href="/static/' + ac.type + '/assets/base.css" rel="stylesheet" type="text/css" />', 'top');
+ ac.assets.addBlob('<!--[if IE]>\n<script src="/static/' + ac.type + '/assets/ie.js"></script>\n<![endif]-->', 'top');
@caridy
caridy Apr 23, 2013

In general, these are things you should not do at the controller level, controller should control the business logic, not the UI. You can move these assets into specs in application.json or into the mojit's defaults.json.

@gvaish
gvaish Apr 23, 2013

Do you mean... part of config and then use ac.config / ac.instance.config?

@gvaish
gvaish Apr 23, 2013

@caridy I can't use that.
The reason is, the ie.js has to be wrapped within a comment - <!--[if IE]> <![endif]-->. I can't do that using static assets config.

@caridy
caridy Apr 23, 2013

That one in particular is more complex to deal with (specially without shaker), but the other two entries, can be moved out using something like this:

        "specs": {
            "frame": {
                "type": "HTMLFrameMojit",
                "config": {
                    "deploy": true,
                    "child": {
                        "type": "foo"
                    },
                    "assets": {
                        "top": {
                            "blob": [
                                "<meta charset='utf-8'>",
                                "<meta http-equiv='X-UA-Compatible' content='IE=edge,chrome=1'>"
                            ]
                        }
                    }
                }
            }
        }
@caridy caridy and 1 other commented on an outdated diff Apr 23, 2013
...examples/mojito/mojits/TodoMojit/controller.common.js
+ Y.namespace('mojito.controllers')[NAME] = {
+
+ index: function(ac) {
+ ac.assets.addBlob('<meta charset="utf-8">\n<meta http-equiv="X-UA-Compatible" content="IE=edge,chrome=1">', 'top');
+ ac.assets.addBlob('<link href="/static/' + ac.type + '/assets/base.css" rel="stylesheet" type="text/css" />', 'top');
+ ac.assets.addBlob('<!--[if IE]>\n<script src="/static/' + ac.type + '/assets/ie.js"></script>\n<![endif]-->', 'top');
+ ac.done({});
+ },
+
+ operate: function(ac) {
+ var op = ac.params.getFromBody('op'),
+ data = ac.params.getFromBody('data'),
+ todo = ac.models.get('Todo');
+
+ Y.log('operate called: op = ' + op + ', todo: ' + todo);
+ switch(op) {
@caridy
caridy Apr 23, 2013

Doug will probably cry over this huge switch! It seems to me that every op in this action should really be an action on its own.

@gvaish
gvaish Apr 23, 2013

Well... I thought about it. Had started with a couple of ops and then thought that would be fine.
Later... was lazy to switch.

I thought of demonstrating the "command pattern" here ;-)

@caridy caridy commented on an outdated diff Apr 23, 2013
...ture-examples/mojito/mojits/TodoMojit/definition.json
@@ -0,0 +1,5 @@
+[
@caridy
caridy Apr 23, 2013

you can remove empty definition.json.

@caridy caridy commented on the diff Apr 23, 2013
...ito/mojits/TodoMojit/tests/controller.common-tests.js
+ assets: {
+ addCss: function(css) {
+ assetsResults = css;
+ },
+ addBlob: function(blob, loc) {
+ }
+ },
+ models: {
+ TodoMojitModelFoo: {
+ getData: function(cb) {
+ cb(null, modelData);
+ }
+ }
+ },
+ done: function(data) {
+ doneResults = data;
@caridy
caridy Apr 23, 2013

Y.Mock is better than trying to store things here and there.

@gvaish
gvaish Apr 23, 2013

Haven't played much with Y.Mock... let me start with it.

@caridy caridy commented on the diff Apr 23, 2013
labs/architecture-examples/mojito/server.js
@@ -0,0 +1,23 @@
+/*
@caridy
caridy Apr 23, 2013

My recommendation is to create a new app, and copy over the new server.js because this one seems a little bit old.

@caridy
caridy commented Apr 23, 2013

Done with my notes! I found nothing big, but few things that could be improved.

+1

@passy
Member
passy commented Apr 23, 2013

Thanks for reviewing, @caridy!

@gvaish
gvaish commented Apr 23, 2013

Thanks a ton @caridy!
Appreciate the review.

I'd be in London Hackathon this weekend... will try to have a push before month-end (can't guarantee though).

@sindresorhus
Member

ping

@gvaish
gvaish commented May 10, 2013

No response from the server.

Service will come back to life next week (not weekend).

@gvaish gvaish added a commit to gvaish/todomvc-mojito that referenced this pull request May 12, 2013
@gvaish gvaish @gvaish Code cleanup as per tastejs/todomvc#507
* application.json: Added assets, cleaned up log
* index.js: Deleted, required only for Manhattan
* definition.json: Empty, removed
* server.js: Updated
db3338f
@gvaish
gvaish commented May 13, 2013

Pls use commit at 2c2b912 for review. f1470cf is empty, a166302 is redundant.

The delta comprises of the changes suggested by @caridy, with the major one being refactoring operate action into individual actions like getById, getAll, delete etc.

@gvaish
gvaish commented May 13, 2013

@sindresorhus Pong! :)

@sindresorhus

"<meta charset=\"utf-8\">",

@sindresorhus

/*jslint anon:true, sloppy:true, nomen:true */

@sindresorhus
Member

@gvaish I can only comment on the code style as I'm not familiar with this framework. Make sure you follow our code style: https://github.com/tastejs/todomvc/blob/gh-pages/codestyle.md

@stephenplusplus @passy anything to add?

@stephenplusplus
Member

I'll be diving into this tonight. From first glance at the readme, getting up and running and the dependency of the other repo seem offputting, though I'm not sure there is a way to avoid this.

@stephenplusplus
Member

I don't want to be a downer as any contribution to TodoMVC is great! However, this seems like more of a "labs" project than an official TodoMVC example. For starters, several files are prefixed with "Copyright (c) 2012 Yahoo! Inc. All rights reserved." However, if this is to be merged, here are just a few suggestions.

As @sindresorhus mentioned, the code style stuff (small things): files currently mix tabs and spaces, rogue indentations, anonymous function() vs anonymous function (). The code style guide will help.

mojits/TodoMojit/assets/ looks like it is intended to hold the todomvc-common files. Please note these have changed, and Bower usage is preferred. Is it possible to use Bower for this dependency and/or any others that the app uses? If not, this folder should be updated to hold the contents from todomvc-common, and include them as well (notably, base.js is missing).

Some functional things:

screen shot 2013-05-13 at 9 23 50 pm

The readme should definitely be expanded upon. Here are some questions and comments about the readme.

Do I have to do the git clone and all of that, or does the TodoMVC repo contain everything I need?

Why do todomvc-mojito-common and todomvc-mojito-server contain the same files?

cd todo-mojito-common; npm install -d

After I clone your repo, there is a "todomvc-mojito-....", but no "todo-mojito-..." Also, what is -d? (Not familiar with that!)

Is there a reason not to include todomvc-mojito-common and todomvc-mojito-server in the TodoMVC repo? To me, it seems like the dependency on your repo is undesirable/unnecessary(?).

In general, I am unfamiliar with Mojito, so I'm a perfect candidate to get use out of this demonstration. In summary, I'm confused as heck!

Thanks for your patience in working on this demo and tolerating my questions. I look forward to learning more!

@gvaish
gvaish commented May 14, 2013

@stephenplusplus Quick response: Have a look at https://github.com/gvaish/todomvc-mojito/ for common vs server thing - they are two different implementations using Mojito.

Detailed response: Framing it.

@passy
Member
passy commented May 14, 2013

@stephenplusplus Excellent as always.

I'll also look over it after the next update. :)

@gvaish
gvaish commented May 23, 2013

Tied up personally. Will take some time...

@passy
Member
passy commented May 23, 2013

@gvaish Thanks for letting us know. :)

@addyosmani
Member

Pinging for a status check :)

@gvaish
gvaish commented Jul 16, 2013

Hate to say, but will have to wait for a couple of weeks more :(
I relocated and now settling down.

@passy
Member
passy commented Jul 16, 2013

@gvaish Thanks for the update!

@sindresorhus
Member

ping

@sindresorhus
Member

closing for lack of activity. We'll reopen when it's ready to be merged. (no need to open a new pr).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment