This repository has been archived by the owner. It is now read-only.

Minimal backbone version #11

Merged
merged 22 commits into from Mar 27, 2013

Conversation

Projects
None yet
4 participants
Member

n1k0 commented Mar 25, 2013

Based on #9, a much lighter approach for a backbone implementation:

  • requirejs is no more used
  • nor handlebars
  • app code fits in a single javascript file
Member

dmose commented Mar 25, 2013

Given our conversations today, I'm assuming that the next step with this PR is to split it up into the refactored talkilla as well as separate PRs for vendor libraries to be landed.

Once that happens, I suspect tOkeshu would be a good choice for the reviewer of record, since he has some past experience there.

n1k0 added some commits Mar 26, 2013

removed dependency to bower
linked statics will be added in another PR

n1k0 added a commit to n1k0/talkilla that referenced this pull request Mar 26, 2013

tOkeshu added a commit that referenced this pull request Mar 26, 2013

Merge pull request #14 from n1k0/backbone-vendor
Added vendor statics related to #11
README.md
@@ -10,16 +10,17 @@ Local Development
2. Make sure you have [node installed](http://nodejs.org/).
-3. Install the required node modules:
+3. Install the required node & [bower](http://twitter.github.com/bower/) dependencies:
@tOkeshu

tOkeshu Mar 26, 2013

Contributor

Maybe it comes from the merges, but we decided to not use bower, right?

@n1k0

n1k0 Mar 26, 2013

Member

Yup, let me fix this.

Contributor

tOkeshu commented Mar 26, 2013

I will be the reviewer of record for this one.

n1k0 added some commits Mar 26, 2013

updated README.md
- removed references to bower
- add make commands
rethinking app modularization
- The Backbone app is now exposed as a `Talkilla` global, to ease further extensibility.
- The main JavaScript file is now stored under a static/js directory
static/js/talkilla.js
+ })
+ .done(function(auth) {
+ if (!auth.nick)
+ return cb(new Error('joining failed'));
@tOkeshu

tOkeshu Mar 27, 2013

Contributor

It seems we don't trust the API here. What I read is « in case we have a successful response but no nick, we have an error ».
I would expect the server to not return a successful response here but something like a 500 or 409

@n1k0

n1k0 Mar 27, 2013

Member

+1, let me address this

CONTRIBUTING.md
+-------------------
+
+The Talkilla Backbone application is exposed as a `Talkilla` global object.
+
@tOkeshu

tOkeshu Mar 27, 2013

Contributor

I'm not sure this should be part of this pull request. What do think @fqueze and @dmose ?

@n1k0

n1k0 Mar 27, 2013

Member

As a side note it's still WiP

For the records I've asked on IRC to B2G/gaia people, and it seems there's no official convention for modularizing app code, though exposing a global object for an app seems to be pretty much a widespread practice…

All in all, I agree it's a bunch of work for this PR which is already quite big, and right now our app fits in a single file so there isn't much to say about code modularization yet, so +1 for postponing it.

@tOkeshu

tOkeshu Mar 27, 2013

Contributor

My concern is more about CONTRIBUTING.md being modified to explain practices.
If I understand well the goal of this pull request is to bring backbone to the project.
Explaining practices should be in another PR IMHO.

@n1k0

n1k0 Mar 27, 2013

Member

Ah, makes sense. We could use the github wiki for that, what do you think?

Edit: I mean, I'm not sure CONTRIBUTING.md should be that long anyway, and much of it could fit better in developer documentation (/methinks)

@dmose

dmose Mar 27, 2013

Member

Totally agree that we should limit the scope of this PR so that it can land sooner rather than later, primarily for the reason that I think it effectively blocks most of the rest of the user stories.

Separately, I agree that CONTRIBUTING.md is too long; I suspect the simplest way to make it better is just to extract the coding stuff stuff into its own MD file. But let's take this discussion to a future PR.

nick = findNewNick(nick);
- res.send(200, JSON.stringify({nick: nick, users: users}));
- users.push(nick);
+ users.push({nick: nick});
@fqueze

fqueze Mar 27, 2013

Contributor

The reason why this was sending the reply before pushing the new nick to the users array is that this way I didn't have to filter the user's nick out of the received list of contacts on the front-end side (offering a user to call himself doesn't make sense).

I haven't searched very hard, but I didn't see any code in the front-end here that filters out the user's nick.

@n1k0

n1k0 Mar 27, 2013

Member

I was thinking of adding no link to the user's own entry adn maybe suffixing it with (me)… a bit like what some irc clients do, but now I'm not so sure :/

@tOkeshu

tOkeshu Mar 27, 2013

Contributor

For now there is no UI to call someone, so it's not a bug (yet). We can decide what to do when the feature will be implemented.
For me it's not a blocker.

@fqueze

fqueze Mar 27, 2013

Contributor

So the "As the first user logged in, I see a message that I am the only person logged in, and see an offer to invite friends" story is no longer done, then?

@n1k0

n1k0 Mar 27, 2013

Member

Indeed! I implemented it in other prs but not in this one. I'll be fixing that right now. Should I create a new PR for the fix?

Contributor

tOkeshu commented Mar 27, 2013

r=tOkeshu conditional on removing the CONTRIBUTING.md patch before you merge.

n1k0 added a commit that referenced this pull request Mar 27, 2013

@n1k0 n1k0 merged commit 444812a into mozilla:master Mar 27, 2013

@n1k0 n1k0 deleted the n1k0:backbone-minimal branch Mar 27, 2013

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.