search

mozilla / SocialShare

Protect users' privacy - share user data only when people are ready to share.
BSD 3-Clause "New" or "Revised" License
27 stars 10 forks source link

Initial Pull Request #1

Closed tallowen closed 12 years ago

tallowen commented 12 years ago

This is everything I've done so far. Please review.

tofumatt commented 12 years ago

It might be worth running http://www.jshint.com/ on the JS code; there are a fair amount of style things it could help you with, and it's a lot more forgiving than JSLint.

tofumatt commented 12 years ago

Also, there seems to be almost every style of variable declarations in the code (foo, fooBar, FooBar, $foo, and foo_bar are all there) -- variable naming could be a bit more consistent (I'm used to seeing methods written fooBar() style, but vars less so.

If there's a method to your madness, please explain and I'll lay off.

fwenzel commented 12 years ago

Please add \.DS_Store to .gitignore and don't check that file in.

fwenzel commented 12 years ago

Done with my code review: I added a few comments.

tofumatt commented 12 years ago

Remove img/.DS_Store from the tree.

tofumatt commented 12 years ago

This is a lot better! Image sprites would rule (I think @fwenzel mentioned it in #5?). But aside from those nits this is great. When you go to make minified versions I assume you'll do sprites too?

fwenzel commented 12 years ago

With Matt and my latest comments, you're good to land this. I suggest you squash some of these commits so you end up with fewer tiny tweaks.