Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

TodoMVC using Mojito #435

Closed
wants to merge 32 commits into from
Closed

TodoMVC using Mojito #435

wants to merge 32 commits into from

Conversation

gvaish
Copy link

@gvaish gvaish commented Feb 12, 2013

TodoMVC using base Mojito framework.

It is missing unit tests, a little difficult to write in YUI.
Mock-tests is what is natively supported in YUI (and hence, Mojito).

…mojito for details)

* mojito/*: TodoMVC using base Mojito framework.
@gvaish gvaish mentioned this pull request Feb 12, 2013
@gvaish
Copy link
Author

gvaish commented Feb 12, 2013

Hmm... looking at the latest screenshot at https://github.com/addyosmani/todomvc (Readme.md), I guess, I'll need to update the assets (specifically, sprite).

Barring that, looks mostly there unless glaring issues are noticed.

@sindresorhus
Copy link
Member

@gvaish Do you know any Mojito people we could get to review?

@gvaish
Copy link
Author

gvaish commented Feb 12, 2013

Adding @rwaldura @caridy @drewfish @isao

Ren, wondering if someone can review the code - officially from the team :)

@caridy
Copy link

caridy commented Feb 12, 2013

@gvaish
Copy link
Author

gvaish commented Feb 13, 2013

@caridy Awesome!

@clarle
Copy link
Contributor

clarle commented Feb 13, 2013

@gvaish Just have a few quick questions:

  • This is your TodoMVC version that doesn't persist information between client and server? I tried running it on two different browsers, and it seems that no information is kept on the server.
  • Are all of the included libraries necessary for the application, and should they be minified? The Mojito roll-up is huge (7,000+ LOC) and is included twice, in /public/static/todomvc and in /src/.

I'll take a more in-depth look at the code in just a little bit, but appreciate your work. :)

@gvaish
Copy link
Author

gvaish commented Feb 14, 2013

  1. Yes. It uses local storage (HTML5)
  2. Rollup has not been minified. But yes, rollup can be large. It can be removed from the src folder. But essentially, it's Mojito runtime - hb/mu compilers, core addons, action-context engine et al.

@rwaldura / @caridy I was under the impression that rollup includes only the referenced ("required") JS even from core-mojito but it seems it contains all of mojito.
Has this behaviour changed in latest version of Mojito - I'm using 0.4.4 presently.

@caridy
Copy link

caridy commented Feb 14, 2013

Two quick notes:

  • upgrade to mojito 0.5.4, that will help a lot for our review.
  • Production readiness comes with shaker, which pushes all the assets into CDN, minified/rolluped up, etc. and I don't think you want to cover that with this prototype, maybe just mention it and link to shaker.

@gvaish
Copy link
Author

gvaish commented Feb 14, 2013

@caridy Closing this request. Let me upgrade to 0.5.4 and send another request.

Production readiness addresses (2).

@clarle Let me know if you have any further queries... Feel free to ping at https://github.com/gvaish/todomvc/issues

@gvaish gvaish closed this Feb 14, 2013
@sindresorhus
Copy link
Member

@gvaish You don't need to open another PR, just push your changes to this PR.

@gvaish
Copy link
Author

gvaish commented Feb 14, 2013

Oh ok.

@gvaish gvaish reopened this Feb 14, 2013
@gvaish
Copy link
Author

gvaish commented Feb 14, 2013

Associating YahooArchive/mojito/issues/982 with this.

Not sure if I am wrong at my end or something else is the issue...

@gvaish
Copy link
Author

gvaish commented Feb 23, 2013

Now that YahooArchive/mojito#982 is closed via YahooArchive/mojito#997, time to update mojito installation and checkout the latest build html5app once again.

…h-pages

Conflicts:
	changelog.md
	labs/dependency-examples/angularjs_require/index.html
@passy
Copy link
Member

passy commented Mar 8, 2013

@gvaish You should rather rebase your commits instead of merging with upstream. Your diff stats are currently "2,450 changed files with 665,995 additions and 0 deletions." :)

Alternatively, you can start a new branch from the current gh-pages and cherry-pick your commits by hand, if that is easier.

@gvaish
Copy link
Author

gvaish commented Mar 12, 2013

@passy I have already done a rebasing in my repo. With the newer version of Mojito (with the required changes) now released, I should be sending an updated pull request by this weekend.

@passy
Copy link
Member

passy commented Mar 12, 2013

Great, looking forward to it!

@sindresorhus
Copy link
Member

@gvaish awesome to hear :)

You can just update this one by force pushing to it.

@passy
Copy link
Member

passy commented Mar 26, 2013

/ping @gvaish :)

@gvaish
Copy link
Author

gvaish commented Mar 28, 2013

/pong Got stuck with Mojito pseudo-hosting - See YahooArchive/mojito#1050.

Here's the final conclusion that I have drawn - cannot host Mojito app on Github - something that I tried to do.
Pushing the code for a pull request in some time.

@isao
Copy link

isao commented Mar 30, 2013

@gvaish looking forward to seeing your app. couple of pointers.

  • if you want a mojito app to be hosted as static files (i.e. no server-side affinities and any kind of httpd) do mojito build html5app Foo
  • hook up the client-side persistance any way you like, if that's a requirement for hosting. I wrote a sample using YUI Storage-Lite that may be helpful.
  • when @passy says "rebase" he means update your todomv branch like git rebase <mainlinebranch> so all the unrelated commits do not appear under a merge commit (see git help rebase first example). Since your branch is not shared yet, you can redo it and git push origin <branchname> --force and that will clean up this PR. Feel free to ping me directly, or find us on irc.

@gvaish
Copy link
Author

gvaish commented Mar 30, 2013

@isao Thanks for the pointers

  1. I did the same mojito build html5app and have common affinities for controller and model. But noticed that for hosting, it still is trying to /tunnel.
  2. Yes. I'm using CacheOffline
  3. I know the rebase part :-) Infact, I had done a rebase before last pull request. Just that I messed up elsewhere and then gave up - and I know giving up using git is just so dumb.

btw, can either you or @caridy review #507.
The main code is in an independent project at https://github.com/gvaish/todomvc-mojito/tree/master/todomvc-mojito-common (controller with common affinity) and https://github.com/gvaish/todomvc-mojito/tree/master/todomvc-mojito-server (controller with server affinity)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.