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

Add Chaplin application. #319

Merged
merged 1 commit into from
Dec 12, 2012
Merged

Add Chaplin application. #319

merged 1 commit into from
Dec 12, 2012

Conversation

paulmillr
Copy link
Contributor

Finally.

Sorry for the delay, bros. Will need to fix shit up to make it conform to the spec a bit but it works. Just give me two days.

@paulmillr paulmillr mentioned this pull request Nov 12, 2012
@addyosmani
Copy link
Member

Thanks for getting this completed @paulmillr! We'll review :)

node_modules/

# Brunch folder for temporary files.
tmp/
Copy link
Member

Choose a reason for hiding this comment

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

remove this file. should be in your global gitignore.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure.

BTW, node_modules needs to be ignored, because it’s what brunch creates for plugins. If any other user will try to build the app, he will see the dir in git repo too.

Copy link
Member

Choose a reason for hiding this comment

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

I agree, just remove everything non-project specific

@paulmillr
Copy link
Contributor Author

thanks for review; I fixed everything.

edit: github is buggy, 66e8ecf ain’t showing somehow

@addyosmani
Copy link
Member

Thanks for working on updating the pull request :) We'll review again.

@sindresorhus
Copy link
Member

Followed the readme instruction, but couldn't get it to work: https://www.evernote.com/shard/s1/sh/cb3e49cf-aad2-4622-9772-aecd735f85e0/135f322cb76eccd51d510b099f5ffc3d

@paulmillr
Copy link
Contributor Author

@sindresorhus that’s because brunch runs webserver on public directory (public/ by default) and lodash etc assets are located higher in fs.

@sindresorhus
Copy link
Member

So how do I get it to work? Can we symlink in the assets into public?

@paulmillr
Copy link
Contributor Author

we can, but i’m just running http server on root directory of todomvc and brunch w (w/o --server) on app directory. works fine

@sindresorhus
Copy link
Member

Ok, sure, let go with the first one, but can you add instruction in the readme on how to do it?

Some comments:

  • New todo:

Make sure to .trim() the input and then check that it's not empty before creating a new todo.

  • Editing:

When editing mode is activated it will hide the other controls and bring forward an input that contains the todo title, which should be focused (.focus()). The edit should be saved on both blur and enter, and the editing class should be removed. Make sure to .trim() the input and then check that it's not empty. If it's empty the todo should instead be destroyed.

  • Counter:

Also make sure to pluralize the item word correctly: 0 items, 1 item, 2 items. Example: 2 items left

  • All filter is not bold on pageload, when no filters are selected
  • Clear completed:

Displays the number of completed todos, and when clicked, removes them. Should be hidden when there are no completed todos.

@addyosmani
Copy link
Member

@paulmillr Thanks once again for working on some of the comments we had! As Sindre mentioned if you could cover any steps needed to get the user up and running with the app (e.g the server) that would save them time trying to figure it out themselves :)

@sindresorhus
Copy link
Member

@paulmillr ping

@addyosmani
Copy link
Member

It would be awesome to get this finished in time for the next release. Fingers crossed Paul will have some bandwidth :)

@paulmillr
Copy link
Contributor Author

All done, please review.

I’d want to squash all commits before merge.

@sindresorhus
Copy link
Member

Also make sure to pluralize the item word correctly: 0 items, 1 item, 2 items. Example: 2 items left

Doesn't seem to have taken. I can see your fix in the commit, but it's not visible when I open index.html.

And #main should be hidden when there are no todos.

Otherwise this looks good to me :)

@paulmillr
Copy link
Contributor Author

@sindresorhus mhm, pluralizing works fine for me. 1 item, 2 items etc (0 items ain’t showing because footer is hidden with 0 items). pushed #main change.

just one detail: can I name the subdirectory as chaplin-brunch instead of chaplin_brunch or there is underscore convention?

@sindresorhus
Copy link
Member

@paulmillr Ah, now I get it. The counter should be updated when a todo is checked/unchecked and not when it's removed.

Dash is fine

@paulmillr
Copy link
Contributor Author

ah, typo. fixed!

sindresorhus added a commit that referenced this pull request Dec 12, 2012
@sindresorhus sindresorhus merged commit 07898f7 into tastejs:gh-pages Dec 12, 2012
@sindresorhus
Copy link
Member

Landed. Awesome stuff 🍔

@addyosmani
Copy link
Member

Thank you so much, Paul! You rock!

@paulmillr
Copy link
Contributor Author

thanks guys. we should work on tastejs!

@sindresorhus
Copy link
Member

Yeah, not possible yet for me though, Yeoman finish-up this week, then 2 weeks Mexico. But after that I'm ready to drive it full on!

@addyosmani
Copy link
Member

We'll setup a TasteJS thread or group to engage with framework authors over the next few weeks. I think Taste won't realistically launch until end of Jan at the earliest, but we can definitely discuss :)

@sindresorhus
Copy link
Member

👍

@reubano
Copy link

reubano commented Apr 27, 2013

@paulmillr I want to make a stand-alone version of this to run with brunch so I copied this example to its own directory and copied the assets over as well. brunch w plus opening the index.html file works. However brunch w -s doesn't. it gives the following error

events.js:66
        throw arguments[1]; // Unhandled 'error' event
                       ^
Error: listen EADDRINUSE
    at errnoException (net.js:769:11)
    at Server._listen2 (net.js:909:14)
    at listen (net.js:936:10)
    at Server.listen (net.js:985:5)
    at Function.app.listen (/opt/local/lib/node_modules/brunch/node_modules/express/lib/application.js:532:24)
    at startDefaultServer (/opt/local/lib/node_modules/brunch/lib/helpers.js:190:12)
    at Object.exports.startServer (/opt/local/lib/node_modules/brunch/lib/helpers.js:213:14)
    at initialize (/opt/local/lib/node_modules/brunch/lib/commands/watch.js:187:26)
    at Object.exports.loadPackages (/opt/local/lib/node_modules/brunch/lib/helpers.js:539:12)
    at initialize (/opt/local/lib/node_modules/brunch/lib/commands/watch.js:161:20)
    at new BrunchWatcher (/opt/local/lib/node_modules/brunch/lib/commands/watch.js:247:7)
    at module.exports.watch (/opt/local/lib/node_modules/brunch/lib/commands/watch.js:282:12)
    at ArgumentParser.parse (/opt/local/lib/node_modules/brunch/node_modules/argumentum/lib/argumentum.js:407:27)
    at Object.exports.run (/opt/local/lib/node_modules/brunch/lib/cli.js:196:47)
    at Object.<anonymous> (/opt/local/lib/node_modules/brunch/bin/brunch:7:32)
    at Module._compile (module.js:449:26)
    at Object.Module._extensions..js (module.js:467:10)
    at Module.load (module.js:356:32)
    at Function.Module._load (module.js:312:12)
    at Module.runMain (module.js:492:10)
    at process.startup.processNextTick.process._tickCallback (node.js:244:9)

mac osx 10.7.5
brunch 1.5.3
npm 1.1.61

any suggestions??

@paulmillr
Copy link
Contributor Author

@reubano it means your 3333 port is taken by other application. try changing port, like brunch w -s -p 3335

@reubano
Copy link

reubano commented Apr 27, 2013

@paulmillr thanks! that was it. works like a charm now.

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.

4 participants