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

Example using TroopJS #120

Closed
wants to merge 12 commits into from
Closed

Example using TroopJS #120

wants to merge 12 commits into from

Conversation

mikaelkaron
Copy link
Contributor

A TodoMVC example using TroopJS

@sindresorhus
Copy link
Member

Thanks for the contribution :D

Can you explain what TroopJS is? Couldn't find any info on it.

@mikaelkaron
Copy link
Contributor Author

Just another front-end framework. Aims to package current javascript technologies and 'bind' them together with minimal effort for the developer.

  • jQuery for DOM operations
  • RequireJS for modularity
  • ComposeJS for object composition
  • Has.js for feature detection

On top of this we've added a pubsub system, templating, weaving (connect widget to dom) and auto wiring.

The TroopJS moto goes something like

Provide as little as possible to do as much as possible

The name is taken from what you'd call a group of monkeys (troop) as originally it was targeted at an 'embedded' environment using spidermonkey.

We're using todomvc as the onboarding project for new developers, so there's a step-by-step guide (in progress) that explains how we created this app.

@mikaelkaron
Copy link
Contributor Author

simplified the file structure quite a bit and provided an example using troopjs-bundle instead of individual modules.

@mikaelkaron
Copy link
Contributor Author

Not really sure what else I can do to get this merged. Perhaps use the new template from #126 when its ready?

@sindresorhus
Copy link
Member

Sorry for the delay. I've been on holiday.

It looks pretty good :)

There are some things I'd appreciate if you could fix:

  • Use the latest template.
    As you've seen, I just landed a new UI. The HTML changes are minimal: You can see the changes in the diff here.
  • Make sure you follow the newly released App Specification.
    Please let me know if anything is unclear or could be improved. It's not field tested yet. The spine.js example, not yet landed, can be used as a reference.
  • This project uses tab indention and follows the Idiomatic guide to writing JavaScript. Please ensure your code follows this closely. If something is unclear, follow the existing style. Use double-quotes in HTML and single-quotes in JS and CSS.

I'll review it more thoroughly when the above changes are implemented :D


Some comments to your excellent TroopJS Todo app intro:

Ours is located in js/lib. As none of the other folders (css, js and img) were pluralized, we thought that it was silly to do it here.

I agree, changed in master.

We do this, but we felt it was natural to do the same when the user removes focus from the input box.

This is specified in the new App Specification.

Since the specification does not define what this checkbox should do when only some of the tasks are marked as completed, we've added an indeterminate state that covers this usecase.

Also in the App Specification.

Do let us know if you have any more feedback or if something is unclear :D

@sindresorhus
Copy link
Member

@mikaelkaron Any updates?

@mikaelkaron
Copy link
Contributor Author

hacking along on 0.0.2 ;) I'll make an updated todomvc as soon as I have that (using the new UI)

@addyosmani
Copy link
Member

@mikaelkaron Thanks for the update! Do you think 0.0.2 might be ready sometime over the next few weeks? Im trying to establish whether you'll have time to bring the TroopJS app in line with specs for our next major version (probably aiming to go out in a month). If you do, I'll mark the target release for this as 1.0 otherwise we can do it in 1.1 (probably 1..a few months after).

We appreciate your contributions!

@mikaelkaron
Copy link
Contributor Author

I'll try to land 0.0.2 ASAP, so please target this for 1.0

@addyosmani
Copy link
Member

Marked accordingly. Let us know if you need any help :)

@sindresorhus
Copy link
Member

@mikaelkaron Any updates? The 1.0 release is close, and we need to get those apps that are going into the release merged.

@mikaelkaron
Copy link
Contributor Author

I've been a bit slow over here. I'll take a stab at it today!

@mikaelkaron
Copy link
Contributor Author

landed a bunch of stuff in my troopjs-todos repo (what I eventually 'copy' to todomvc). basically implemented all the requirements except routing. will take a stab at that tomorrow.

@mikaelkaron
Copy link
Contributor Author

a bit of reset and a commit later. done.


.filter-active .completed,
.filter-completed .active {
display: none;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CSS shouldn't be tied to the filters functionality.

Instead you should just hide/show the items directly.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just trying to save myself some coding for when you have a filter applied and add/remove items. I'll take a look at if it can be done without CSS.

@sindresorhus
Copy link
Member

Thanks for finishing this. I have to say, I like to good use of $.Deferred ;)

I've done a review of your app and have some comments in addition to the inline ones:

  • Move it to the labs/architecture-examples folder. Remember to update all the references in the HTML.

Develop in a topic branch (not master) and submit against the labs folder in the master branch

  • Counter should say e.g: 2 items left
  • When in editing mode and remove the todo contents and hit enter, i get this error msg:

Uncaught Error: NOT_FOUND_ERR: DOM Exception 8

  • Drop the animation stuff
  • Use the jQuery in the assets folder
  • Minified:

Use minified version of third-party code and put it in the lib folder.

  • Too much use of unnessecary constants

@mikaelkaron
Copy link
Contributor Author

I'll get on the CSS update tomorrow. Do you want me to make a topic branch for all of this as per:

Develop in a topic branch (not master) and submit against the labs folder in the master branch

or are you ok with this pull request as it is (besides the outstanding CSS fix)?

I was also unable to reproduce your error, do you still have it?

@sindresorhus
Copy link
Member

I'll get on the CSS update tomorrow. Do you want me to make a topic branch for all of this as per:

No not necessary, but keep it in mind for your next pull request. It makes your life a lot easier if you've ever need to submit multiple pull requests to the same project.

  • I've just added require.js to our assets folder. Can you use that instead? (Note that it's Require 2.0)

I was also unable to reproduce your error, do you still have it?

Yes, I still have that issue, Chrome 19 Mac Lion:

Steps: Double-click the item, remove all the text, hit enter

Uncaught Error: NOT_FOUND_ERR: DOM Exception 8 jquery.min.js:4
f.fn.extend.remove jquery.min.js:4
onDelete list.js:139
b troopjs-bundle.js:11
f.event.dispatch jquery.min.js:3
f.event.add.h.handle.i jquery.min.js:3
f.event.trigger jquery.min.js:3
f.fn.extend.trigger jquery.min.js:3
e.extend.each jquery.min.js:2
e.fn.e.each jquery.min.js:2
f.fn.extend.trigger jquery.min.js:3
q troopjs-bundle.js:11
f.event.dispatch jquery.min.js:3
f.event.add.h.handle.i jquery.min.js:3
f.event.trigger jquery.min.js:3
f.fn.extend.trigger jquery.min.js:3
e.extend.each jquery.min.js:2
e.fn.e.each jquery.min.js:2
f.fn.extend.trigger jquery.min.js:3
r troopjs-bundle.js:11
f.event.dispatch jquery.min.js:3
f.event.add.h.handle.i jquery.min.js:3
f.event.trigger jquery.min.js:3
f.fn.extend.trigger jquery.min.js:3
e.extend.each jquery.min.js:2
e.fn.e.each jquery.min.js:2
f.fn.extend.trigger jquery.min.js:3
f.each.f.fn.(anonymous function) jquery.min.js:3
onCommitFocusOut list.js:208
b troopjs-bundle.js:11
f.event.dispatch jquery.min.js:3
f.event.add.h.handle.i jquery.min.js:3
f.event.trigger jquery.min.js:3
f.fn.extend.trigger jquery.min.js:3
e.extend.each jquery.min.js:2
e.fn.e.each jquery.min.js:2
f.fn.extend.trigger jquery.min.js:3
q troopjs-bundle.js:11
f.event.dispatch jquery.min.js:3
f.event.add.h.handle.i jquery.min.js:3
f.event.trigger jquery.min.js:3
f.fn.extend.trigger jquery.min.js:3
e.extend.each jquery.min.js:2
e.fn.e.each jquery.min.js:2
f.fn.extend.trigger jquery.min.js:3
r troopjs-bundle.js:11
f.event.dispatch jquery.min.js:3
f.event.add.h.handle.i jquery.min.js:3
f.event.trigger jquery.min.js:3
f.fn.extend.trigger jquery.min.js:3
e.extend.each jquery.min.js:2
e.fn.e.each jquery.min.js:2
f.fn.extend.trigger jquery.min.js:3
f.each.f.fn.(anonymous function) jquery.min.js:3
onCommitKeyUp list.js:194
b troopjs-bundle.js:11
f.event.dispatch jquery.min.js:3
f.event.add.h.handle.i jquery.min.js:3
f.event.trigger jquery.min.js:3
f.fn.extend.trigger jquery.min.js:3
e.extend.each jquery.min.js:2
e.fn.e.each jquery.min.js:2
f.fn.extend.trigger jquery.min.js:3
q troopjs-bundle.js:11
f.event.dispatch jquery.min.js:3
f.event.add.h.handle.i jquery.min.js:3
f.event.trigger jquery.min.js:3
f.fn.extend.trigger jquery.min.js:3
e.extend.each jquery.min.js:2
e.fn.e.each jquery.min.js:2
f.fn.extend.trigger jquery.min.js:3
r troopjs-bundle.js:11
f.event.dispatch jquery.min.js:3
f.event.add.h.handle.i jquery.min.js:3

Other than that, it looks good :)

@mikaelkaron
Copy link
Contributor Author

Ok, I'm able to reproduce in chrome. I'll take a look at that now.

@mikaelkaron
Copy link
Contributor Author

that requirejs 2.0 thing sort of threw a wrench into my schedule (had to do some updates to the bundle to support 2.0). I have a version of the bundle that is 2.0 compatible, but as there are still quite a few bugs with 2.0 and rjs I'd like to stick to 1.0 if possible.

@sindresorhus
Copy link
Member

that requirejs 2.0 thing sort of threw a wrench into my schedule (had to do some updates to the bundle to support 2.0). I have a version of the bundle that is 2.0 compatible, but as there are still quite a few bugs with 2.0 and rjs I'd like to stick to 1.0 if possible.

Didn't mean to make a lot of extra work for you. Noticed it while surfing and it looked like and easy upgrade. But sure, stick with 1.0 if that's more stable ;)

Just make sure to also minify the require.js file.

@addyosmani
Copy link
Member

1.0 should be okay for now :) I know James Burke mentioned there's still a few weeks of work left to wrap up 2.0 so we should be okay to upgrade at a later point as needed.

@sindresorhus
Copy link
Member

@addyosmani Actually 2.0 has been released.

@addyosmani
Copy link
Member

@sindresorhus Oh wow! That I did not know. He must have had a few late nights to wrap it all up so quickly :)

@mikaelkaron
Copy link
Contributor Author

There. Fixed what needed to be fixed to be RequireJS 2.0 'compatible'.

@sindresorhus
Copy link
Member

Cool, just need that error fixed and we're golden ;)

@mikaelkaron
Copy link
Contributor Author

Fixed.

Funny problem there actually, it's jQuery that triggers a simulated focusout event on the containing input as I .remove() the li, thereby re-triggering the whole chain (I click() a corresponding .destroy when the input is empty and we lose focus)

@sindresorhus
Copy link
Member

Right, when I think about it I think we had another app with the same problem, turned out to actually be a Chrome bug.

Landed. Thanks for this addition to our collection ;)

@mikaelkaron
Copy link
Contributor Author

Well, I was only able to reproduce in chrome, and the underlying error is that jQuery will try to remove a node that does not exist anymore. I guess it's possible that this is the same problem (were you removing an element that had input descendants?).

@sindresorhus
Copy link
Member

Can't remember, but here is the Chromium bug: http://code.google.com/p/chromium/issues/detail?id=104397

And here's the jQuery bug: http://bugs.jquery.com/ticket/11663

@mikaelkaron
Copy link
Contributor Author

looks like the same to me. nice to know ppl are aware about it.

@sindresorhus
Copy link
Member

But unfortunately not a lot of activity on the Chrome ticket...

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

Successfully merging this pull request may close these issues.

3 participants