Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

JavaScriptMVC labs upgrade #252

Closed
wants to merge 32 commits into from

4 participants

@juristr

I tried to upgrade the example of the JavaScriptMVC framework, adding

  • upgrading the codebase to the latest JavaScriptMVC v3.2.2
  • adding routing support
  • upgrade to latest TodoMVC styles

The steal-compiled production version is linked from the production.html file while the "code-expanded" version is referenced from the index.html file (as usual).

Fixes #66

@addyosmani
Owner

Thanks @juristr!.

@justinbmeyer @ralphholzmann would you guys be interested in taking a look at whether these updates are in line with your best practices? We'll be reviewing how well it matches our specs, but just thought I'd check :)

@juristr

@justinbmeyer @ralphholzmann

Btw, I structured the app using controllers, models and views folder. I know that this changed according to your documentation in v3.2.2, however I thought it would be easier for jmvc newbies to understand it when comparing it with other frameworks like Backbone, Spine etc.

Maybe I should mention this in the readme.md doc.

@sindresorhus
Owner

Thanks :)

Some comments:

  • Would it be possible to include JavaScriptMVC Steal as single files? The JSMVC app is half the projects size...

  • If possible, please use the base.css and bg.png in the /assets folder. Also, destroy.png can be removed.

  • If I check a todo when on the Active filter, the selected filter is suddenly moved to All.

  • If I check a todo, and then click "Mark all as complete", the previously check todo looses it's stroke-through.

  • 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.

  • The localStorage key name should be: todos-javascriptmvc (with an s)

  • Routing:

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.

  • production.html: Place the comment at the bottom. Nothing should be above the doctype.

  • Routing should be #/active or #!/active, not #active.

  • Please make sure it adheres to our template. I see some missing stuff in the HTML.

@justinbmeyer

So, why is folder arrangement important to TodoMVC? JMVC has (what I would argue) is a more evolved pattern focused on reusability. For example, it tries to localize css and images to the widget they are used with instead of housing them in a global assets folder.

Also is "#/" really important?

All of this can be fixed, I'm just wondering why this is part of the spec. Thanks!

@sindresorhus
Owner

It isn't. We only suggest a structure, you're free to choose whatever fits the framework best practices. We have the base.css in the assets folder so not to have 40 copies of it in every app.

The hash change is only for consistency.

@juristr

hmm...apparently some bugs got into the codebase during the last refactoring. I'll try to fix them up in the coming days over the weekend.
Thx for the feedback.

@juristr

So, why is folder arrangement important to TodoMVC? JMVC has (what I would argue) is a more evolved pattern focused on reusability. For example, it tries to localize css and images to the widget they are used with instead of housing them in a global assets folder.

@justinbmeyer No it's not that the new JavaScriptMVC structure is less evolved or not suitable. It's just that most other TodoMVC frameworks use a model, view, controller separation in terms of folders. So I thought it might be easier to compare the frameworks if they reflect the same kind of folder structure.
Using the JavaScriptMVC recommended structure, how would you structure it in terms of the Todo app?

todo
  |_ todolist
        |_ models
              |_ todo.js (containing the todo model + todo.list model
        |_ views (containing the list + list entry views)
        |_ todolist.js (containing the controller for the list)
   |_ todo.js (the app itself, also containing the router
   |_ index.html

@sindresorhus What is your view? Would such widget-based organization be fine for TodoMVC? Btw, what exactly is missing in the HTML file??

@sindresorhus
Owner

@juristr I would go for a JSMVC idiomatic folder structure.

Reference to ie.js and base.js is missing in index.html.`

@juristr

@sindresorhus oh, ok. Seems like I had an old version of backbone todomvc sample which didn't include those. Btw, backbone doesn't include base.js neither. So it's missing there as well I suppose.

@sindresorhus
Owner

@juristr Yup, and that's exactly why we want people to base their implementations on the template ;) Fixed now.

@sindresorhus
Owner

Also make sure you bind editing mode to the <label> element. This was changed recently in the spec.

@sindresorhus

This is ready to be reviewed if either of you would take a look @justinbmeyer @ralphholzmann ?

@juristr
@addyosmani
Owner

Thanks for the update @juristr. We appreciate you taking another look at the app :)

@juristr

Ok, finally found the time to reorganize the structure of the app to match the one suggested by JavaScriptMVC.

It would be cool if you could review my changes and make some comments/suggestions, @sindresorhus @justinbmeyer @ralphholzmann

thx

@sindresorhus

@sindresorhus Single quotes for all strings in JavaScript?

Yup, the only way ;)

@sindresorhus

this.find('#main, #footer').toggle( list.length );

@addyosmani
Owner

I think we'll wait until someone at Bitovi (e.g @justinbmeyer, @ralphholzmann) have time to review this, otherwise its just little tweaks remaining.

@addyosmani
Owner
@justinbmeyer

@sindresorhus sorry about being so slow on this. A new client has me working day and night. I'll pull it tomorrow and look it over on my home from cupertino.

@justinbmeyer

Btw, what am I reviewing?

There's a few things I might change:

1

steal.css('todo')
     .plugins('jquery/model/list',
        'jquery/controller',
        'jquery/view/ejs',
        'jquery/lang/json')

This seems to be with an older version of JMVC (<3.2). For 3.2, it should look like:

steal('./todo.css',
   'jquery/model/list',
   'jquery/controller',
   'jquery/view/ejs',
   'jquery/lang/json',
   function($){
...
})

Upgrading to 3.2 would be awesome.

2

FuncUnit is not included, but funcunit tests are included. I'm not sure what to do about that.

3

As CanJS is really JMVC without steal, and the very similar CanJS example looks almost the same as this, it maybe best to show this with JMVC's typical folder structure (I remember talking about this before and forgot why it was decided to got the single file route). But a more standard folder structure would look like:

todo
   \todo.js - steals todo/todos and todo/models/todo and runs $("#todos").todos({list : new Todo.List()});
   \todos
      \todos.js - Creates the Todos controller
   \models
     \todo.js - Creates the todo model and model list.

I'll keep looking it over, but please let me know if this is what you guys want me to look at?

@juristr

@justinbmeyer Are you sure you didn't look at an old version?? The one of my pull request should use v3.2, i.e. there shouldn't be any steal.plugins(...) etc.

Moreover what I tried to achieve is the "widget-like" structuring suggested in v3.2, namely

  • a todoitem widget (representing a single todo list entry)
  • a todolist widget (representing the list of todoitems and according logic)
  • the app itself, which uses both, todoitem and todolist and handles the glue stuff like routing, entering new items etc..

Could you take a look at my pull request and add your comments on whether my changes are appropriate or not. Thx a lot.

@sindresorhus

@justinbmeyer @juristr Also, I would like to minimize the file bloat. Right now there a way too many files. We'd like to only include what's absolutely necessary if possible. Can any of the dependencies/buildscript be concatenated to one file, etc?

@justinbmeyer

Can you use submodules? That way you don't have to have any of the framework code.

This will work better in the long run because I'd like to show how JMVC can build, test, and document the app (it's a big feature of the framework ... otherwise people can just look at the CanJS examples).

@sindresorhus

@justinbmeyer We could, but the problem is that submodules aren't included when downloading zipball, which most of our users do...

@juristr

@sindresorhus @justinbmeyer I agree with Justin, one of the reason for chosing JMVC in our company was its full-stack characteristic, namely test, docs, building etc...Maybe this should be highlighted somehow to the user, also to justify the presence of all of the files. In the README.md maybe?

@sindresorhus

Would it be possible to have a simple make file that fetched the dependencies with curl or something?

@justinbmeyer

If that can download the steal.zip, funcunit.zip, etc and unzip them ... yeah, that could work.

@addyosmani
Owner

Would it be possible to use https://github.com/jeresig/pulley or something similar to squash the commits on this? Ralph was going to review but there are too many files changes across different commits atm.

@juristr

@addyosmani I know, it's due to the reorganization that was suggested by @justinbmeyer as it's the way JMVC v3.2 is intended to work. That resulting in a restructuring of the folders and I had to move lots of code pieces around. That said it's probably easier to directly look at the entire jmvc-labs-upgrade branch. I guess for @justinbmeyer or @ralphholzmann it shouldn't be too hard to spot potential problems.

Plz let me know if I can help in some way.

@sindresorhus

@juristr As Addy commented, can you squash your commits? http://gitready.com/advanced/2009/02/10/squashing-commits-with-rebase.html

What do you think about my suggestion on fetching the dependencies with curl?

@juristr

@sindresorhus

As Addy commented, can you squash your commits? http://gitready.com/advanced/2009/02/10/squashing-commits-with-rebase.html

Never did that, but I'll give it a try as soon as I find a free spot.

What do you think about my suggestion on fetching the dependencies with curl?

That could work, by having some kind of init script or something. The problems I see there:

  • under a Windows system? We would have to create an alternative or the user needs to have unix tools installed s.t. we can exec a wget or something
  • how would the version work online on todomvc.com? We could run it in "production mode" which would imply however that the user could not inspect the source code directly online but would have to download it from the repo. Or I could try to remove all the resources that are not necessarily needed for running (but just for building, documenting, testing etc...). @justinbmeyer What's your opinion?
@sindresorhus

under a Windows system? We would have to create an alternative or the user needs to have unix tools installed s.t. we can exec a wget or something

For Windows we'll just explain how to get the dependencies manually

juristr added some commits
@juristr juristr Upgrade of the JMVC todo app to v3.2 including the reorganization of …
…the app using the suggested "widget" approach;

implemented routing support and adapted the style to the last TodoMVC one
7c8ef5f
@juristr juristr Merge branch 'jmvc-labs-upgrade' of github.com:juristr/todomvc into j…
…mvc-labs-upgrade
197adeb
@juristr

Ok, so as @sindresorhus and @addyosmani suggested, I tried to squash my commits. Hope everything worked as expected as it was the first time I used the command.

Many commits/changes are due to the upgrade of javascriptmvc. So basically everything under steal or jquery should not be considered.

Review info:

To summarize, what I did in order to upgrade the JMVC todo app is

  • upgrading JMVC itself
  • upgrade to the latest CSS changes (referencing base.css, updating the base HTML skeleton)
  • added routing support (which was not present previously) including all the features that are consequences of the routing (i.e. the filters etc...)
  • fixed some bugs I found in the old version
  • reorganized the existing/my new code to match the structure suggested by @justinbmeyer namely a widget-like structure where todoitem, todolist and the app are somewhat more independent and separated modules which mainly communicate through events.

So to speak, especially the reorganization of the app resulted in a lot of small diffs wherefore I'm not sure whether it would be probably easier to directly take a look at my repo where I made the changes and the app itself rather than purely reviewing the commit diffs.

@addyosmani
Owner

@justinbmeyer @ralphholzmann sorry to bug you again, but do you think you could review the updated PR? We've able to comment on general JavaScript/app code quality, but you guys are the real experts when it comes to JMVC.

@sindresorhus

I would like to see us finish up this PR. Ping pong didely dong.

@addyosmani
Owner

@sindresorhus If we're unable to get a JMVC team review on this in the next week, lets merge. It's in Labs and can be improved on further later as needed.

@sindresorhus

@juristr Still not squashed.

That could work, by having some kind of init script or something.

How about a simple Node.js script that fetches the deps?

@juristr

Still not squashed.

@sindresorhus Sure, it should be, isn't it. At least I followed the suggestions you gave me earlier to squash the commits. But again, I have the feeling that for this upgrade it is probably better to directly take a look at the sources in my repo rather than going to review the diffs. There is too much stuff that has been moved around/rewritten due to structural changes between JMVC v3.1 and v3.2.

To what I didn't yet look at was the script for handling the numerous JavaScriptMVC dependencies. I'll take a look at that. Currently I have the idea of using a submodules approach s.t. at least those fetching the sources with GIT have already setup everything and then to add some kind of (preferably OS-agnostic) script that fetches the dependencies as already discussed earlier.

@sindresorhus

Currently I have the idea of using a submodules

Submodules won't work as they aren't included in GitHubs zipballs...

@juristr

Yep, I know, @addyosmani already mentioned that. That's why I thought of using submodules for those fetching the src via Git and a script for the remaining ones downloading the src as zip files.

I just thought that might be more convenient.

@sindresorhus

Oh, ok. I would prefer to keep it consistent and submodule free if we can, so a Python/Ruby/Node script would probably be the best solution.

@juristr

Ok. I'll see what I can do.

@addyosmani addyosmani referenced this pull request from a commit
@addyosmani addyosmani Bringing back in JavaScriptMVC implementation based on #252. Note: we…
… need to reduce JMVC dep. bloat.
75b9d74
@sindresorhus sindresorhus referenced this pull request from a commit
@sindresorhus sindresorhus Revert "Bringing back in JavaScriptMVC implementation based on #252. …
…Note: we need to reduce JMVC dep. bloat."

This reverts commit 75b9d74.

Conflicts:
	index.html
7a7e64e
@addyosmani
Owner

It looks like Bitovi have just pushed updates to their JavaScriptMVC TodoMVC app over at https://github.com/bitovi/todomvc-javascriptmvc (as per https://twitter.com/javascriptmvc/status/329790225326944260). @sindresorhus is it worth us exploring these updates?

@sindresorhus
Owner

I'm not sure I see the point. It was removed for a reason. Though I'm happy to be convinced otherwise :)

@gustaff-weldon gustaff-weldon referenced this pull request from a commit in gustaff-weldon/todomvc
@addyosmani addyosmani Bringing back in JavaScriptMVC implementation based on #252. Note: we…
… need to reduce JMVC dep. bloat.
1ed07fa
@gustaff-weldon gustaff-weldon referenced this pull request from a commit in gustaff-weldon/todomvc
@sindresorhus sindresorhus Revert "Bringing back in JavaScriptMVC implementation based on #252. …
…Note: we need to reduce JMVC dep. bloat."

This reverts commit 75b9d74.

Conflicts:
	index.html
ab7a096
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Jul 30, 2012
  1. @juristr
  2. @juristr

    started to refactor model, controller and views into separate folders…

    juristr authored
    … according to the jmvc guidelines
  3. @juristr
Commits on Jul 31, 2012
  1. @juristr

    added official jmvc release 3.2.2 (instead of latest from github as t…

    juristr authored
    …hat appears to be unstable right now)
  2. @juristr
  3. @juristr
  4. @juristr
  5. @juristr

    toggle-all works now

    juristr authored
  6. @juristr
  7. @juristr

    some cleanup + fixed a bug

    juristr authored
Commits on Aug 1, 2012
  1. @juristr

    adjusted view references

    juristr authored
  2. @juristr

    changed the structure to better reflect the mvc structure (although t…

    juristr authored
    …he official docs of v3.2 propose a slightly different organization)
  3. @juristr
Commits on Aug 2, 2012
  1. @juristr
  2. @juristr
  3. @juristr
  4. @juristr

    just some aesthetic fixes

    juristr authored
  5. @juristr

    fixed localstorage name

    juristr authored
Commits on Aug 5, 2012
  1. @juristr

    fix: routing with #!/...

    juristr authored
  2. @juristr
  3. @juristr

    routing fix in prod html file

    juristr authored
  4. @juristr
  5. @juristr
  6. @juristr

    fix: moved comment to bottom

    juristr authored
  7. @juristr

    added ie.js fix

    juristr authored
Commits on Aug 6, 2012
  1. @juristr
Commits on Aug 31, 2012
  1. @juristr

    refactored the whole application and reorganized the code to match th…

    juristr authored
    …e suggested JMVC app structure
  2. @juristr
  3. @juristr
  4. @juristr
Commits on Sep 26, 2012
  1. @juristr

    Upgrade of the JMVC todo app to v3.2 including the reorganization of …

    juristr authored
    …the app using the suggested "widget" approach;
    
    implemented routing support and adapted the style to the last TodoMVC one
  2. @juristr
Something went wrong with that request. Please try again.