Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Review comments: Chapter 7 - Backbone Extensions #339

Closed
dcmaf opened this Issue · 21 comments

5 participants

@dcmaf
Collaborator

Section Boilerplate Rendering Code

  1. MyModel is used as the model type in the example but is not defined anywhere.

Section: Memory Management

  1. States that the problem occurs because we pass this.render as the callback, giving the model a direct reference to the view instance. Since render is just a function, isn't the issue that we are passing 'this' as the context to the on method? Are there not two separate issues (one being a memory leak for the view object because it is being held the context for the first callback binding, the other being that the first binding is not removed before the second instance is created)? Obviously, removing the first callback resolves both issues, but the explanation will be confusing if it is not accurate.

  2. the listenTo() API in 0.9.10 could help somewhat, but the issue of not implicitly removing the replaced view would remain. May want to revise this section to reflect this.

Section: Marionette Todo App

  1. In the second figure (the Todo App snapshot with the regions shaded) it would be helpful to overlay the jQuery selectors on each region to better indicate the purpose of the snapshot (shouldn't the "What needs to be done?" line be part of the header section?)

  2. Some graphics depicting the relationship between Layouts, Regions, etc. would be helpful.

  3. Might want to have the RequireJS chapter before this one since it makes reference to RequireJS/AMD.

  4. It would be helpful to show the HTML being referenced, even though it is similar to that in the first exercise. I think this version contains ids (#header, etc.) which are not in the first.

  5. In TodoMVC.Layout.js, Layout.Header extends Marionette.ItemView and Layout.Footer extends Marionette.Layout. Should Layout.Header extend Marionette.Layout? If not, some explanation of why the Layout is directly extending ItemView should be provided.

  6. Some discussion of the content of TodoMVC.Layout.js would be helpful. Something along the lines of the Marionette-specific features being used to achieve the same effects as the original Todo (e.g., using ui, bindTo, etc.).

  7. Add discussion of what is happening in TodoMVC.TodoList.js.

  8. Similarly, add discussion for Router/Controller and View code examples, focusing on how the Marionette implementation differs from our original version and the benefits of using Marionette.

Section: Is the Marionette implementation of the Todo app more maintainable?

  1. This section gets a little heavy on the design principle/pattern abstractions. A picture here showing the relationship between the high-level Marionette components would help make it more tangible.

Section: Thorax

  1. Recommend splitting Thorax into its own chapter.

Section: Hello World

  1. This section notes how Thorax.View differs from Backbone.View in that it does not have an options object. I don't think the View's options object has been discussed previously so it either needs to be introduced here or in an earlier chapter.

Section: Embedding child views

  1. "Or the name of a child view to initialize (and any optional properties to pass)." seems like a sentence fragment.

  2. I'm having a hard time following what is going on with Thorax. A more concrete example from a user's perspective would be helpful (is the this.$el.find(...) inside Thorax or is that something the user writes?)

@addyosmani
Owner

Thanks for reading through this, @dcmaf!

@derickbailey, could you let me know what you had in mind for myModel? I was going to just replace it with a generic Backbone Model constructor but if there was a specific Model type we had in mind here, I'm happy to update.

@derickbailey

This was likely copy and paste or something else that needs to be changed, for the my model thing.there are a lot of other good points here, and I need to update the section on marionette to reflect a lot of recent changes. I'll get that done as soon as I can

@addyosmani
Owner

Thanks @derickbailey. That would be hugely appreciated :)

@addyosmani
Owner

@jsoverson do you think you might have any time to help us with the marionette updates? Help from someone close to the project would really help us keep this section accurate.

@jsoverson

I'll take a look, but I've been out of dedicated browser development for a couple months. I've been focusing mostly on grunt and the build process for marionette but can see if I can help out.

Might be worth poking @mbriggs too.

@jsoverson

What, specifically, should be focused on, or is this a general request?

@addyosmani
Owner

It would be of tremendous help if someone familiar with the current version of Marionette could read through the chapter on it and let us know what specifically might need to be updated. Even better if they could help with a PR :)

@derickbailey

Hey guys - sorry I haven't been able to get this done yet. We have a release coming up on the 20th and I'm pulling my hair out getting screencasts and presentations ready for it. Once I get past these (this week), I'll be able to get back to re-writing this chapter if no one else is able to get to it first.

@addyosmani
Owner

cc @eastridge regarding the comments on the Thorax chapter.

@eastridge

@addyosmani I'm ok with splitting up Thorax into it's own chapter. @dcmaf I'd really like to address your concerns about the Thorax chapter, but can you be more specific? What sections are tough? What code examples don't make sense? Really appreciate you taking the time to give such detailed feedback about the book.

~ Ryan

@dcmaf
Collaborator

@eastridge I'm reading back through it now - here are some notes:

  • The stated intent of the chapter is to show patterns used in Thorax that will be helpful to developers even if they don't use Thorax. I think it would help set the context to outline what these are in the intro section (i.e., view helpers, custom HTML data attributes).

  • I think a large part of the problem I had reading the code is that I haven't used Handlebars templates and all the examples in the book to this point have used Underscore Microtemplates.

  • The Embedding child views section starts talking about "the view helper". I think this is where I started getting lost because I found myself going "what view helper?" Now that I am looking at it again, I think the intent here is to introduce the concept of view helpers, in which case "A view helper" would, I think, make that clearer.

  • In the Embedding child views, View Helper, and collection helper sections, I think it would help to have more discussion of the problems that these helpers are solutions for (i.e., why would I want/need to do something like this). A more tangible example (e.g., table/row, form/input) rather than an abstract parent/child might also help.

  • In its current state, these helper view sections feel more like documentation of how to use Thorax (e.g., different ways to create child views) rather than presenting patterns that can be applied using generic Backbone. I think that, in order to illustrate a pattern, they also need detailed explanations of what is happening in the code examples (including explaining what is going on in the Handlerbars templates for those of us who aren't familiar with it).

  • The Custom HTML data attributes section gets back on topic by discussing how any Backbone project can use custom data attributes and giving examples of doing so in non-Thorax code, along with illustrating some things you do with them in Thorax. Again, detailed discussion of the code examples would be helpful, especially in this case for readers who may not be familiar with calling base class methods through a prototype or adding functions to jQuery. Even if it feels like you are stating the obvious, it may not be to the reader or it may make it easier for the reader to digest.

  • In the anti-pattern example at the end of the Custom HTML data attributes section, it would be helpful to have the example show both what not to do and what to do instead, something along the lines of:

/* instead of */
.child {
...
}

/* use */
[data-view-name="child"] {
...
}
  • The "add a discussion" comment applies to the Todo MVC example in the Thorax Resources section as well. It might also be helpful to have some discussion of what the difference is between Thorax's Model, View, and Collection and their Backbone counterparts. This could be done up-front in the Hello World section. I could see that section as being an introduction to Thorax and what it is about, with the remainder of the chapter focusing on the patterns it uses and how they can be applied in any Backbone application.
@eastridge

Wow, thanks @dcmaf. Fantastic feedback. @addyosmani I don't have time to address this week, but will be pushing some updated copy next week.

@addyosmani
Owner

@eastridge Thank you. I greatly appreciate your time on the updates :)

@derickbailey Once again, thank you so much for the updates. I don't want to take any more of your time up, but in https://github.com/addyosmani/backbone-fundamentals/blob/gh-pages/chapters/06-extensions.md MyModel and ZombieView are used in an example without being defined. I assume MyModel could just be:

var MyModel = Backbone.Model.extend({
  defaults: {
    "firstName": "Jeremy",
    "lastName": "Ashkenas",
    "email":    "jeremy@example.com"
  }
});

or something similar.

@derickbailey

which example did I miss? thought i had all of them covered with the right model and view definitions... but, yes, that would be it basically. feel free to edit as you see fit, or point me to where the problem is and I'll adjust it.

@addyosmani
Owner

Reading through once again it looks like you did cover ZombieView afterall - it was just the model definition missing. Thanks for confirming that should be okay. I'll update accordingly!

@addyosmani
Owner

ping @eastridge :)

@addyosmani
Owner

I believe Derick has addressed most of the Marionette issues mentioned here (minus #387) but I've just tried to address two of the points made about the Thorax chapter. I feel like we could still use some further clarifications to the line regarding objects (as cited in the feedback) if that's possible.

@raDiesle raDiesle referenced this issue from a commit in raDiesle/backbone-fundamentals
@addyosmani For #339: adding missing model definition, updating examples to more …
…readable vars
36fedb4
@raDiesle raDiesle referenced this issue from a commit in raDiesle/backbone-fundamentals
@addyosmani For #339: some clarifications for Thorax section 13b9d7b
@addyosmani
Owner

@dcmaf As we're aiming to hand in the manuscript tomorrow, it's looking like we may have to punt on further changes to the Thorax section for the next version of the book. I'm currently reviewing your last set of feedback and it appears this chapter may require some more extensive re-writing to address all points.

I feel like we could ship the first version with Thorax as-is but update as soon as @eastridge has more time. Does that sound reasonable?

@eastridge

@addyosmani sorry I have not been available. I can push a set of changes Monday night PST, that's about 36 hours from now. Does that work for you?

@addyosmani
Owner

@eastridge That would be fantastic. O'Reilly use on-demand printing so we would include the updates as soon as they get merged and I send them over. Thanks for any time you can spend on this.

@dcmaf
Collaborator

Sounds reasonable. I'm working my way through the updated mobile chapter and should have some updates checked in for that by tomorrow morning (GMT).

@addyosmani addyosmani closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.