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

refactoring wires and bootstrap headers in preparation for search #73

Open
wants to merge 2,444 commits into
base: master
Choose a base branch
from
Open

Conversation

pehrlich
Copy link

@pehrlich pehrlich commented Nov 4, 2011

In this commit:

  • moved bootstrap header code to one common partial
    -- created helpers for various external JS (such as exceptional)
  • re-factored thoroughly and converted all wires to coffee script
  • removed options.account and options.community from wires and resources (clean!)
  • added sinatra reloader and commented it out again (caused slowdowns)

Upcoming changes:

  • implementing search feature requests
  • fix bug where reloading changes the url and doesn't select the tab
  • experiment with making event handling declarative in the DOM
  • mixpanel API standardization

Changes to be done eventually:

  • refractor landing resources and community resources and convert to coffee
  • see wires/base.js
  • exploration in to why sinatra reloader causes rails requests to take 100x the time.

CODE REVIEW REQUESTED
almost every page on the site has been touched by these changes!

  • check source
  • check over pages, make sure titles are correct, no errors from missing resources, and functionality is correct
  • please excuse occasional foreign references to poop.

New practices, demonstrated in this code (I hope!):

  • CommonPlace.community = community;
    • window.current_account = new Account(#{serialize(Account.new(current_user))});

use these always, and replace where you can. options.account and options.community are now DEPRECATED

We have too big a project not to be always improving it. Every code you touch, make it better, delete stuff, and make things simpler and more DRY. An important note here is not to put effort in to improving code you're not expanding upon. First it can only break things, and second when expansion does need to happen, you may find you've done the wrong improvements.

  • coffeescript is really important for our own programming speed and ability. Learn it, use it, love it.
    -- backbone extend is inferior to the coffeescript implementation. read their docs. Functionally, coffeescript allows the super() method and chained initializers. Aesthetically, the implementation is much easier to understand, both in debugging and writing. See base/wire.js.coffee for detail on how extension needs to be done. (specifically, Backbone.View.apply is important)
    -- be warned that file line numbers are not accurate with coffeescript

WHEN CODING think about how it reads.

  • Pick names and keep them accurate, doing no more or less than the name implies.
  • Anytime you use a function and it doesn't read like a sentence, it is a hack. Either fix it or label it as such.
  • TODOs and fixmes left in are ok! We've got shit to do, places to go. But these will make incremental improvement much faster as our system grows.
  • why separate events from views? Because code should be thought of to work robustly in any context, not only of the event you're currently writing it for.
  • never ever push code that has anything repeated. Even jQuery selectors.
    -- creates self-documentation
    -- reduces typos
    -- reduces reading comprehension to one location
    -- reduces reading comprehension time
    -- creates clear intent
    -- becomes safe to change and build upon
    -- creates names for things in the debugger

PS: Thoughts on haml? Oppositions to erb? I am bothered a lot when I can't indent my javascripts or multiline my erg (such as including partials.)

PPS: I would like to highlight one bit of coffeescript, taken from their docs. @ indicated this., => keeps the context.

  Account = (customer, cart) ->
    @customer = customer
    @cart = cart

  $('.shopping_cart').bind 'click', (event) =>
    @customer.purchase @cart

* get '/feeds/[id]/owners'
* post '/feeds/[id]/owners?emails=[list_of_emails]'
* delete '/feeds/[feed_id]/owners/[feed_owner_id]'
@maxtilford
Copy link

what's up with zother_wires?

@pehrlich
Copy link
Author

pehrlich commented Nov 8, 2011

I believe I made a comment. Inclusion order. It's not perfect, but
functional not permanent. I'm hoping a good solution will emerge.

Sent from my iPhone

On Nov 7, 2011, at 8:44 PM, Max Tilford
[email protected]
wrote:

what's up with zother_wires?


Reply to this email directly or view it on GitHub:
#73 (comment)

@pehrlich
Copy link
Author

pehrlich commented Nov 8, 2011

I think the other reason for the indeterminate state may be the unsure
future of said other wires. Maybe they'll be deleted and the file
will hold one class and be renamed.

Sent from my iPhone

On Nov 7, 2011, at 8:44 PM, Max Tilford
[email protected]
wrote:

what's up with zother_wires?


Reply to this email directly or view it on GitHub:
#73 (comment)

@pehrlich
Copy link
Author

pehrlich commented Nov 9, 2011

da da da da da

@Jberlinsky Jberlinsky force-pushed the master branch 7 times, most recently from 9320072 to 77d6828 Compare February 17, 2015 20:04
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.

5 participants