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

Update emberjs example #63

Closed
boushley opened this issue Jan 18, 2012 · 28 comments
Closed

Update emberjs example #63

boushley opened this issue Jan 18, 2012 · 28 comments
Assignees
Milestone

Comments

@boushley
Copy link
Contributor

  1. Need to update the html/css/folder structure to match the new template.
  2. Update to the latest version of the libraries.
@boushley
Copy link
Contributor Author

Other deficiencies:

  • Clicking on the task content should not mark the task as completed.
  • Double click should allow editing.
  • "Mark all complete" checkbox should be hidden when there are no tasks.
  • Footer should be hidden when there are no tasks.
  • Need to add delete ability on hover over tasks.

@ghost ghost assigned addyosmani Jan 19, 2012
@sindresorhus
Copy link
Member

The Ember+Require app just landed in 1b65322

@stas Would you be interested in getting the non-require version completed? I think there could be a good amount of reuse.

//cc @addyosmani

@stas
Copy link
Contributor

stas commented Apr 30, 2012

I could take a look if there's nobody else interested in picking it up.
Also maybe we should ask @tomdale
He is the original author and It would be nice to ask him first.

@tomdale
Copy link
Contributor

tomdale commented Apr 30, 2012

👍

@addyosmani
Copy link
Member

I think the non-AMD/Require version wouldn't be too difficult to complete. Always happy for @tomdale to review anything Ember related going into this project, just to be on the safe side :)

@sindresorhus
Copy link
Member

@stas Still interested? We would really like to get an up to date version into our 1.0 release 😃

@stas
Copy link
Contributor

stas commented May 28, 2012

Hey, sorry guys I'm a bit busy lately.
I was following ember devlog and was thinking to update both examples with the new routing support.

When you were planning to release 1.0, lets see if we can sync.

@sindresorhus
Copy link
Member

Np, I absolutely understand ;)

We've set the release date to june 20th, which means we'll need it some days before that.
Do you think you'll be able to have it done by that?

@stas
Copy link
Contributor

stas commented May 29, 2012

20th looks great, my holidays start early june, so it seems doable.
Sure this should not stop people jumping in :)

@sindresorhus
Copy link
Member

@stas Sounds good! :D

@addyosmani
Copy link
Member

I've got bandwidth to work on this so I'm going to wrap it up this evening and hopefully have @sindresorhus review :)

@ghost ghost assigned addyosmani Jun 4, 2012
@sindresorhus
Copy link
Member

If you dare :p

@addyosmani
Copy link
Member

I plan on submitting this while you're asleep.

It's the safest way to sneak in a merge ;D

@stas
Copy link
Contributor

stas commented Jun 4, 2012

Sounds cool :)

@sindresorhus
Copy link
Member

I never sleep! :trollface:

@addyosmani
Copy link
Member

Of course you don't :p

One does not simply

@addyosmani
Copy link
Member

@sindresorhus could you review https://github.com/addyosmani/todomvc/tree/emberjs?

If you're happy with it, let me know and I'll pull it into master, otherwise address any issues you notice :)

@sindresorhus
Copy link
Member

Prepare... Ya, there are a few comments :p

  • Trim input on edit and remove if empty

Make sure to .trim() the input and then check that it's not empty. If it's empty the todo should instead be destroyed.

  • Remove app.css and the reference in the HTML
  • app.js needs an IIFE and 'use strict'
  • app.js: Missing some statement curly braces
  • Split app.js up in appropriate files?

@addyosmani
Copy link
Member

@sindresorhus thanks for the review. I'll address those!

On the last point, I think we need to decide whether we want to enforce splitting up of applications or keeping it optional. I was looking at the Backbone app last night and remember that it currently also keeps all of its logic in a single file (but of course can be split out). Thoughts?

@sindresorhus
Copy link
Member

Even though not necessary in our small Todo app, I think we should enforce some good practices.

@tomdale
Copy link
Contributor

tomdale commented Jun 17, 2012

+1 on splitting into multiple files. I think that making these as "realistic" as possible is important. Every web app developer I know is using a tool for file management these days.

@addyosmani
Copy link
Member

Updated in https://github.com/addyosmani/todomvc/tree/emberjs:

  • IIFE + use strict added
  • Split app.js into its component parts (models, controllers, main)
  • Missing curly braces added
  • Wasn't sure about the app.css comment - it's used in the Spine demo app and is needed for overrides

Missing: Trim input on edit and remove if empty

@tomdale there's currently one issue with the app that I'm trying to wrap my head around. We're working under the same namespaces as the RequireJS version of the app, however, now that IIFEs are being used the binding context for content seems to be a little messed up. e.g the remaining and completed item counts now only seem to update correctly on page refresh but not when the app is interacted with.

Do you think you might be able to take a look to see what I'm doing wrong? :)

@stas
Copy link
Contributor

stas commented Jun 25, 2012

There's a new pull request #202 to include routing support.

@addyosmani please check if there are any other issues like the one with counts.

@addyosmani
Copy link
Member

@sindresorhus I've just patched the trim on edit behaviour and the remove if empty should be working fine too. Could you do a final review on the emberjs branch so I can merge into master if we're happy with it? :)

@sindresorhus
Copy link
Member

@addyosmani Almost, only thing left is that the filters are not selected when clicked. All should be selected by default. The others should be selected when the hash changes and on load.

@sindresorhus
Copy link
Member

ping

@addyosmani
Copy link
Member

@sindresorhus Tried fixing this, but ran into some roadblocks. I've pinged a few Ember devs including @tomdale but I think they might be swamped. If no one is able to help resolve, I'll give this another shot tomorrow morning.

@sindresorhus
Copy link
Member

Closing as routing was fixed in #231

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

No branches or pull requests

5 participants