Issue 11013043: View slots implementation
DescriptionView slots implementation
Viewlet Container can define slots, a mapping of logical slot name
to CSS selector within the ViewletContainer DOM.
Viewlets can define slot, the logical name within the container to fill.
showViewlet takes an optional model when showing a viewlet with a slot. This
will remove any old bindings associated with the viewlet and bind in the new
model.
https://code.launchpad.net/~bcsaller/juju-gui/viewlet-slots/+merge/173604
(do not edit description out of merge proposal)
Patch Set 1 #
Total comments: 26
Patch Set 2 : View slots implementation #
Total comments: 37
Patch Set 3 : View slots implementation #Patch Set 4 : View slots implementation #Patch Set 5 : View slots implementation #
Total comments: 19
Patch Set 6 : View slots implementation #
MessagesTotal messages: 16
Please take a look.
Sign in to reply to this message.
This is looking really good thanks for putting in the time! I feel that any viewlet properties should be defined in ViewletBase in view-container.js so that they are properly documented. This will then require the checks for these properties to be more resilient. https://codereview.appspot.com/11013043/diff/1/app/views/databinding.js File app/views/databinding.js (right): https://codereview.appspot.com/11013043/diff/1/app/views/databinding.js#newco... app/views/databinding.js:217: viewlet.remove = Y.bind(function() { niiiice https://codereview.appspot.com/11013043/diff/1/app/views/databinding.js#newco... app/views/databinding.js:226: viewlet._events = []; this should probably be set in view-container.js in ViewletBase then this check is not necessary. https://codereview.appspot.com/11013043/diff/1/app/views/databinding.js#newco... app/views/databinding.js:396: // TODO: Filter bindings removing any where viewlet === bindging.viewlet. maybe a note to why this is required? https://codereview.appspot.com/11013043/diff/1/app/views/view-container.js File app/views/view-container.js (right): https://codereview.appspot.com/11013043/diff/1/app/views/view-container.js#ne... app/views/view-container.js:198: this.viewletConfig = options.viewlets; not necessary for this branch but we should really document all of these properties with @property so that we have proper API docs for this class. https://codereview.appspot.com/11013043/diff/1/app/views/view-container.js#ne... app/views/view-container.js:207: // that the model will change. I don't believe this comment is accurate any longer. https://codereview.appspot.com/11013043/diff/1/app/views/view-container.js#ne... app/views/view-container.js:238: extra line https://codereview.appspot.com/11013043/diff/1/app/views/view-container.js#ne... app/views/view-container.js:240: if (typeof this.template === 'string') { oops :) https://codereview.appspot.com/11013043/diff/1/app/views/view-container.js#ne... app/views/view-container.js:245: // We may want to make this selector user defined at some point can be removed, it is user defined. https://codereview.appspot.com/11013043/diff/1/app/views/view-container.js#ne... app/views/view-container.js:250: if (!viewlet.name) { name by default is '' in ViewletBase so this check is unnecessary or at least inadequate. https://codereview.appspot.com/11013043/diff/1/app/views/view-container.js#ne... app/views/view-container.js:253: if (viewlet.slot) { This should be defined as a default in ViewletBase and then have a proper check here. https://codereview.appspot.com/11013043/diff/1/app/views/view-container.js#ne... app/views/view-container.js:257: if (result && typeof result === 'string') { This can stay in as it's a good idea to have but I'm curious as to why this is required now. Are there instances where we don't return a constructed element? https://codereview.appspot.com/11013043/diff/1/app/views/view-container.js#ne... app/views/view-container.js:284: viewlet.model = model; If this is called as a event callback then model in this case will actually be the event object and I'm going to guess will cause issues in fillSlot() no? https://codereview.appspot.com/11013043/diff/1/app/views/view-container.js#ne... app/views/view-container.js:291: Called automatically by showViewlet when the viewlet has a it's actually always called by showViewlet https://codereview.appspot.com/11013043/diff/1/app/views/view-container.js#ne... app/views/view-container.js:312: if (model === undefined) { because showviewlet could be called as an event callback this could be an object and still not be a model.
Sign in to reply to this message.
Thanks for the review, pushing changes next. https://codereview.appspot.com/11013043/diff/1/app/views/databinding.js File app/views/databinding.js (right): https://codereview.appspot.com/11013043/diff/1/app/views/databinding.js#newco... app/views/databinding.js:226: viewlet._events = []; On 2013/07/08 22:46:57, jeff.pihach wrote: > this should probably be set in view-container.js in ViewletBase then this check > is not necessary. Done. https://codereview.appspot.com/11013043/diff/1/app/views/databinding.js#newco... app/views/databinding.js:226: viewlet._events = []; On 2013/07/08 22:46:57, jeff.pihach wrote: > this should probably be set in view-container.js in ViewletBase then this check > is not necessary. Done. https://codereview.appspot.com/11013043/diff/1/app/views/databinding.js#newco... app/views/databinding.js:396: // TODO: Filter bindings removing any where viewlet === bindging.viewlet. On 2013/07/08 22:46:57, jeff.pihach wrote: > maybe a note to why this is required? I'm not sure that it is. If the template changed somehow in response to the re-rendering of a showViewlet with a new model its render method could have added (and removed) some of the bindings. Cleaning out the bindings map would ensure we're not trying to update elements no longer in the DOM. Thats the thinking. Currently though I think there is little harm w/o this. Open to discussion. https://codereview.appspot.com/11013043/diff/1/app/views/view-container.js File app/views/view-container.js (right): https://codereview.appspot.com/11013043/diff/1/app/views/view-container.js#ne... app/views/view-container.js:207: // that the model will change. On 2013/07/08 22:46:57, jeff.pihach wrote: > I don't believe this comment is accurate any longer. removed. https://codereview.appspot.com/11013043/diff/1/app/views/view-container.js#ne... app/views/view-container.js:238: On 2013/07/08 22:46:57, jeff.pihach wrote: > extra line Done. https://codereview.appspot.com/11013043/diff/1/app/views/view-container.js#ne... app/views/view-container.js:245: // We may want to make this selector user defined at some point On 2013/07/08 22:46:57, jeff.pihach wrote: > can be removed, it is user defined. Done. https://codereview.appspot.com/11013043/diff/1/app/views/view-container.js#ne... app/views/view-container.js:250: if (!viewlet.name) { On 2013/07/08 22:46:57, jeff.pihach wrote: > name by default is '' in ViewletBase so this check is unnecessary or at least > inadequate. That is no longer an acceptable default then. This maps the name from the configuration into the object properties, I think we still want something like this. https://codereview.appspot.com/11013043/diff/1/app/views/view-container.js#ne... app/views/view-container.js:253: if (viewlet.slot) { On 2013/07/08 22:46:57, jeff.pihach wrote: > This should be defined as a default in ViewletBase and then have a proper check > here. > It is now, but this check is saying if it is defined skip it meaning we don't rendered viewlets that expect a slot by default. showViewlet must be called for those. https://codereview.appspot.com/11013043/diff/1/app/views/view-container.js#ne... app/views/view-container.js:257: if (result && typeof result === 'string') { On 2013/07/08 22:46:57, jeff.pihach wrote: > This can stay in as it's a good idea to have but I'm curious as to why this is > required now. Are there instances where we don't return a constructed element? This is really to support custom render functions which you have another test for. Our render function sets viewlet.container, but the signature used for render methods in the test is to return a string. https://codereview.appspot.com/11013043/diff/1/app/views/view-container.js#ne... app/views/view-container.js:284: viewlet.model = model; On 2013/07/08 22:46:57, jeff.pihach wrote: > If this is called as a event callback then model in this case will actually be > the event object and I'm going to guess will cause issues in fillSlot() no? Ahh, nice catch, yes. However in those cases model will not be defined so I can check that condition around the assignment. https://codereview.appspot.com/11013043/diff/1/app/views/view-container.js#ne... app/views/view-container.js:291: Called automatically by showViewlet when the viewlet has a On 2013/07/08 22:46:57, jeff.pihach wrote: > it's actually always called by showViewlet Yeah, and then it returns most of the time, but I'll update the prose. https://codereview.appspot.com/11013043/diff/1/app/views/view-container.js#ne... app/views/view-container.js:312: if (model === undefined) { On 2013/07/08 22:46:57, jeff.pihach wrote: > because showviewlet could be called as an event callback this could be an object > and still not be a model. The model checks here were moved up into showViewlet and seem sane enough for now.
Sign in to reply to this message.
Please take a look.
Sign in to reply to this message.
THIS IS NOT A FULL REVIEW. PLEASE, SOMEONE ELSE, REVIEW THIS BRANCH. Hi Ben. This looks good. I have some niggles that are important to me for readability, but they won't change the big picture. I didn't get to the fillSlot stuff--I ran out of time. See you Wednesday. Gary https://codereview.appspot.com/11013043/diff/7001/app/views/databinding.js File app/views/databinding.js (right): https://codereview.appspot.com/11013043/diff/7001/app/views/databinding.js#ne... app/views/databinding.js:188: @param {Object||Array} viewlet or array of viewlets. trivial and old, but the code would read better if the argument were "viewlets" https://codereview.appspot.com/11013043/diff/7001/app/views/databinding.js#ne... app/views/databinding.js:192: var modelEvents = this.unbindModel(model); On first reading, my reaction is to wonder why we unbind the model as soon as we are asked to bind with it, and then to wonder why mutating the result of the call (in the "push" line below) is useful. Maybe it will make more sense as I read more, but at first glance I don't like the way it reads. At the very least this needs some nice comments. Perhaps it also needs some renaming, or refactoring? Hopefully I'll have a more fully-formed opinion after I read more. https://codereview.appspot.com/11013043/diff/7001/app/views/databinding.js#ne... app/views/databinding.js:197: Y.each(viewlet, function(v, name) { you don't use "name": delete? https://codereview.appspot.com/11013043/diff/7001/app/views/databinding.js#ne... app/views/databinding.js:225: if (viewlet.rebind) { I still don't like this method name. I think something explicit would be much better, such as getBindModel or getTarget or findModel or findBindModel or findTarget... https://codereview.appspot.com/11013043/diff/7001/app/views/databinding.js#ne... app/views/databinding.js:229: // When rebind doesn't return a valid target, skip the binding. I guess this is a feature we need. I can't help but wonder if we ought to write something to the console in case this was unintentional, or make using this feature more explicit (catching a specific error, for instance). At the least, if the comment is accurate, the conditional should go within the previous block; and if it is not, the comment should be corrected. https://codereview.appspot.com/11013043/diff/7001/app/views/databinding.js#ne... app/views/databinding.js:259: // are currently read-only so this work ok. trivial typo: ...so this works ok. https://codereview.appspot.com/11013043/diff/7001/app/views/databinding.js#ne... app/views/databinding.js:362: var mID = model.get('id'); I worry about our need for POJO support. It's coming in the unit view, yeah? Because that information can change dynamically. And then we have a bit of thinking to do? That's an aside: I don't expect this branch to make the necessary changes here or elsewhere. https://codereview.appspot.com/11013043/diff/7001/app/views/databinding.js#ne... app/views/databinding.js:363: var modelEvents = this._models[mID] || []; The attr should be called _modelEventHandles or similar. https://codereview.appspot.com/11013043/diff/7001/app/views/databinding.js#ne... app/views/databinding.js:371: this._models[mID] = modelEvents; This and the next line don't belong in a method that is called "unbindModels" IMO. This is the root of my unhappiness with the "bind" changes. I'd much prefer to have this functionality handled with a "addEventHandleForModel" method, or at least a "getEventHandlesForModel" or something. The name and approach is negotiable, but I don't think the current approach reads well. https://codereview.appspot.com/11013043/diff/7001/app/views/databinding.js#ne... app/views/databinding.js:383: var events = viewlet._events; They are not _events, they are _eventHandles. https://codereview.appspot.com/11013043/diff/7001/app/views/databinding.js#ne... app/views/databinding.js:385: return; If you continue to return an empty list of event handles at the end of this method, you should do so here as well. However, I'd prefer it if you kept this and also did not return event handles at the end of the method. https://codereview.appspot.com/11013043/diff/7001/app/views/databinding.js#ne... app/views/databinding.js:392: // TODO: Filter bindings removing any where viewlet === bindging.viewlet. It would be nice if the comment clarified why: as far as I can tell we always return an empty list. Also, typo: bindging -> binding. https://codereview.appspot.com/11013043/diff/7001/app/views/databinding.js#ne... app/views/databinding.js:393: return events; As with unbindModel, I request that we not do this. https://codereview.appspot.com/11013043/diff/7001/app/views/view-container.js File app/views/view-container.js (right): https://codereview.appspot.com/11013043/diff/7001/app/views/view-container.js... app/views/view-container.js:151: Model change events associated with this viewlet. event handles
Sign in to reply to this message.
Hi Ben, I like the work you've done but agree with the suggestions Gary has made as to the naming changes. I'd like to look at the branch again after his changes are incorporated. It looks very close, though. https://codereview.appspot.com/11013043/diff/7001/app/views/view-container.js File app/views/view-container.js (right): https://codereview.appspot.com/11013043/diff/7001/app/views/view-container.js... app/views/view-container.js:268: } Add a comment about why a viewlet with a slot is skipped. https://codereview.appspot.com/11013043/diff/7001/app/views/view-container.js... app/views/view-container.js:288: @param {String} viewletName is a string representing the viewlet name. Add docstring for model. https://codereview.appspot.com/11013043/diff/7001/app/views/view-container.js... app/views/view-container.js:328: if (existing) { I'd move this inside the previous block, since it cannot happen otherwise when outside. https://codereview.appspot.com/11013043/diff/7001/test/test_view_container.js File test/test_view_container.js (right): https://codereview.appspot.com/11013043/diff/7001/test/test_view_container.js... test/test_view_container.js:216: // The old model (still associated with the viewContianer) isn't bound typo: viewContainer https://codereview.appspot.com/11013043/diff/7001/test/test_view_container.js... test/test_view_container.js:220: }); Nice narrative.
Sign in to reply to this message.
Thanks for review, I did rename a number of methods hoping to make things a little more clear. https://codereview.appspot.com/11013043/diff/7001/app/views/databinding.js File app/views/databinding.js (right): https://codereview.appspot.com/11013043/diff/7001/app/views/databinding.js#ne... app/views/databinding.js:188: @param {Object||Array} viewlet or array of viewlets. On 2013/07/09 01:57:07, gary.poster wrote: > trivial and old, but the code would read better if the argument were "viewlets" Done. https://codereview.appspot.com/11013043/diff/7001/app/views/databinding.js#ne... app/views/databinding.js:192: var modelEvents = this.unbindModel(model); On 2013/07/09 01:57:07, gary.poster wrote: > On first reading, my reaction is to wonder why we unbind the model as soon as we > are asked to bind with it, and then to wonder why mutating the result of the > call (in the "push" line below) is useful. Maybe it will make more sense as I > read more, but at first glance I don't like the way it reads. At the very least > this needs some nice comments. Perhaps it also needs some renaming, or > refactoring? Hopefully I'll have a more fully-formed opinion after I read more. I now call these methods resetModelChangeEvents and resetViewletDOMEvents, hope that helps. It is a bit odd perhaps that I return the list which can be appended to rather than resort to another lookup in the calling function. If that bothers you I can change it, but I don't think its that complex given the better naming. https://codereview.appspot.com/11013043/diff/7001/app/views/databinding.js#ne... app/views/databinding.js:197: Y.each(viewlet, function(v, name) { On 2013/07/09 01:57:07, gary.poster wrote: > you don't use "name": delete? Done. https://codereview.appspot.com/11013043/diff/7001/app/views/databinding.js#ne... app/views/databinding.js:225: if (viewlet.rebind) { On 2013/07/09 01:57:07, gary.poster wrote: > I still don't like this method name. I think something explicit would be much > better, such as getBindModel or getTarget or findModel or findBindModel or > findTarget... selectBindModel(model) -> model now, better? https://codereview.appspot.com/11013043/diff/7001/app/views/databinding.js#ne... app/views/databinding.js:229: // When rebind doesn't return a valid target, skip the binding. On 2013/07/09 01:57:07, gary.poster wrote: > I guess this is a feature we need. I can't help but wonder if we ought to write > something to the console in case this was unintentional, or make using this > feature more explicit (catching a specific error, for instance). At the least, > if the comment is accurate, the conditional should go within the previous block; > and if it is not, the comment should be corrected. This occurs in tests where service models might not have a units lazymodellist. I don't think we want console warning in those cases and we don't want to have to construct more state to test these things but if this still bugs you we can refactor some tests down the road. https://codereview.appspot.com/11013043/diff/7001/app/views/databinding.js#ne... app/views/databinding.js:259: // are currently read-only so this work ok. On 2013/07/09 01:57:07, gary.poster wrote: > trivial typo: > ...so this works ok. Done. https://codereview.appspot.com/11013043/diff/7001/app/views/databinding.js#ne... app/views/databinding.js:362: var mID = model.get('id'); On 2013/07/09 01:57:07, gary.poster wrote: > I worry about our need for POJO support. It's coming in the unit view, yeah? > Because that information can change dynamically. And then we have a bit of > thinking to do? > > That's an aside: I don't expect this branch to make the necessary changes here > or elsewhere. I'd add an identity callback returning the id if/when we actually need this. https://codereview.appspot.com/11013043/diff/7001/app/views/databinding.js#ne... app/views/databinding.js:363: var modelEvents = this._models[mID] || []; On 2013/07/09 01:57:07, gary.poster wrote: > The attr should be called _modelEventHandles or similar. Done. https://codereview.appspot.com/11013043/diff/7001/app/views/databinding.js#ne... app/views/databinding.js:371: this._models[mID] = modelEvents; On 2013/07/09 01:57:07, gary.poster wrote: > This and the next line don't belong in a method that is called "unbindModels" > IMO. This is the root of my unhappiness with the "bind" changes. > > I'd much prefer to have this functionality handled with a > "addEventHandleForModel" method, or at least a "getEventHandlesForModel" or > something. The name and approach is negotiable, but I don't think the current > approach reads well. Hopefully the new naming subsumes these comments. https://codereview.appspot.com/11013043/diff/7001/app/views/databinding.js#ne... app/views/databinding.js:383: var events = viewlet._events; On 2013/07/09 01:57:07, gary.poster wrote: > They are not _events, they are _eventHandles. Done. https://codereview.appspot.com/11013043/diff/7001/app/views/databinding.js#ne... app/views/databinding.js:385: return; On 2013/07/09 01:57:07, gary.poster wrote: > If you continue to return an empty list of event handles at the end of this > method, you should do so here as well. However, I'd prefer it if you kept this > and also did not return event handles at the end of the method. _events is managed automatically now and doesn't need to be checked for https://codereview.appspot.com/11013043/diff/7001/app/views/databinding.js#ne... app/views/databinding.js:392: // TODO: Filter bindings removing any where viewlet === bindging.viewlet. On 2013/07/09 01:57:07, gary.poster wrote: > It would be nice if the comment clarified why: as far as I can tell we always > return an empty list. Also, typo: bindging -> binding. I responded to jeff about this one, should be in the reply. https://codereview.appspot.com/11013043/diff/7001/app/views/view-container.js File app/views/view-container.js (right): https://codereview.appspot.com/11013043/diff/7001/app/views/view-container.js... app/views/view-container.js:151: Model change events associated with this viewlet. On 2013/07/09 01:57:07, gary.poster wrote: > event handles Done. https://codereview.appspot.com/11013043/diff/7001/app/views/view-container.js... app/views/view-container.js:268: } On 2013/07/09 12:58:56, bac wrote: > Add a comment about why a viewlet with a slot is skipped. Done. https://codereview.appspot.com/11013043/diff/7001/app/views/view-container.js... app/views/view-container.js:288: @param {String} viewletName is a string representing the viewlet name. On 2013/07/09 12:58:56, bac wrote: > Add docstring for model. Done. https://codereview.appspot.com/11013043/diff/7001/app/views/view-container.js... app/views/view-container.js:328: if (existing) { On 2013/07/09 12:58:56, bac wrote: > I'd move this inside the previous block, since it cannot happen otherwise when > outside. Done. https://codereview.appspot.com/11013043/diff/7001/test/test_view_container.js File test/test_view_container.js (right): https://codereview.appspot.com/11013043/diff/7001/test/test_view_container.js... test/test_view_container.js:216: // The old model (still associated with the viewContianer) isn't bound On 2013/07/09 12:58:56, bac wrote: > typo: viewContainer Done. https://codereview.appspot.com/11013043/diff/7001/test/test_view_container.js... test/test_view_container.js:220: }); On 2013/07/09 12:58:56, bac wrote: > Nice narrative. Thanks
Sign in to reply to this message.
Please take a look.
Sign in to reply to this message.
Please take a look.
Sign in to reply to this message.
Please take a look.
Sign in to reply to this message.
Thanks for the great work! LGTM So many fixes in here but they were worth it!. I'd like to see a follow-up with tests to check these fixes pretty soon. I'd also like some additional commenting (possibly as a follow-up) about the dragons in this code.
Sign in to reply to this message.
LGTM. I had a few comments for your consideration. https://codereview.appspot.com/11013043/diff/24001/app/views/databinding.js File app/views/databinding.js (right): https://codereview.appspot.com/11013043/diff/24001/app/views/databinding.js#n... app/views/databinding.js:360: @return {Array} of modelEventHandles (empty but appendable). I like the way the two lines above read but I don't think this style of comment will render well. https://codereview.appspot.com/11013043/diff/24001/app/views/databinding.js#n... app/views/databinding.js:365: if (modelEventHandles.length > 0) { Wouldn't the code have the same effect without this conditional? https://codereview.appspot.com/11013043/diff/24001/app/views/databinding.js#n... app/views/databinding.js:394: BindingEngine.prototype.getViewlet = function(name) { Missing comment. https://codereview.appspot.com/11013043/diff/24001/app/views/view-container.js File app/views/view-container.js (right): https://codereview.appspot.com/11013043/diff/24001/app/views/view-container.j... app/views/view-container.js:224: this._slots = {}; // {String} slot: viewlet. I think I see what these comments are saying but more words would probably increase the odds that a reader will understand them. https://codereview.appspot.com/11013043/diff/24001/app/views/view-container.j... app/views/view-container.js:243: be called for them to render. Slots are typical filled through event s/typical/typically/ https://codereview.appspot.com/11013043/diff/24001/app/views/view-container.j... app/views/view-container.js:256: if (typeof this.template === 'string') { The fact that this bug slipped through suggests that there was a missing test. It would be nice to add one that would have failed. https://codereview.appspot.com/11013043/diff/24001/app/views/view-container.j... app/views/view-container.js:288: @param {Model} model to associate with the viewlet in its slot. I think we normally capitalize these descriptions. https://codereview.appspot.com/11013043/diff/24001/app/views/view-container.j... app/views/view-container.js:315: @param {Model} to bind to the slot. More capitalization and phrasing to check for sane rendering. https://codereview.appspot.com/11013043/diff/24001/test/test_view_container.js File test/test_view_container.js (right): https://codereview.appspot.com/11013043/diff/24001/test/test_view_container.j... test/test_view_container.js:184: viewContainer.render(); I would have an assertion that shows that the viewlet was not rendered before .render() was called.
Sign in to reply to this message.
Thanks for the reviews, make changes and card as needed. Submitting in a sec. https://codereview.appspot.com/11013043/diff/24001/app/views/databinding.js File app/views/databinding.js (right): https://codereview.appspot.com/11013043/diff/24001/app/views/databinding.js#n... app/views/databinding.js:360: @return {Array} of modelEventHandles (empty but appendable). On 2013/07/09 19:56:23, benji wrote: > I like the way the two lines above read but I don't think this style of comment > will render well. Done. https://codereview.appspot.com/11013043/diff/24001/app/views/databinding.js#n... app/views/databinding.js:365: if (modelEventHandles.length > 0) { On 2013/07/09 19:56:23, benji wrote: > Wouldn't the code have the same effect without this conditional? Done. https://codereview.appspot.com/11013043/diff/24001/app/views/databinding.js#n... app/views/databinding.js:394: BindingEngine.prototype.getViewlet = function(name) { On 2013/07/09 19:56:23, benji wrote: > Missing comment. Done. https://codereview.appspot.com/11013043/diff/24001/app/views/view-container.js File app/views/view-container.js (right): https://codereview.appspot.com/11013043/diff/24001/app/views/view-container.j... app/views/view-container.js:224: this._slots = {}; // {String} slot: viewlet. On 2013/07/09 19:56:23, benji wrote: > I think I see what these comments are saying but more words would probably > increase the odds that a reader will understand them. Done. https://codereview.appspot.com/11013043/diff/24001/app/views/view-container.j... app/views/view-container.js:256: if (typeof this.template === 'string') { On 2013/07/09 19:56:23, benji wrote: > The fact that this bug slipped through suggests that there was a missing test. > It would be nice to add one that would have failed. Added this to the 'more tests' card https://codereview.appspot.com/11013043/diff/24001/app/views/view-container.j... app/views/view-container.js:288: @param {Model} model to associate with the viewlet in its slot. On 2013/07/09 19:56:23, benji wrote: > I think we normally capitalize these descriptions. I was using the formal param names, I think that makes sense? https://codereview.appspot.com/11013043/diff/24001/app/views/view-container.j... app/views/view-container.js:315: @param {Model} to bind to the slot. On 2013/07/09 19:56:23, benji wrote: > More capitalization and phrasing to check for sane rendering. added a word or two, but not sure if we should titlecase formal param names. https://codereview.appspot.com/11013043/diff/24001/test/test_view_container.js File test/test_view_container.js (right): https://codereview.appspot.com/11013043/diff/24001/test/test_view_container.j... test/test_view_container.js:184: viewContainer.render(); On 2013/07/09 19:56:23, benji wrote: > I would have an assertion that shows that the viewlet was not rendered before > .render() was called. That is done in the preceding test but I can add it here as well.
Sign in to reply to this message.
Good stuff. https://codereview.appspot.com/11013043/diff/24001/app/views/view-container.js File app/views/view-container.js (right): https://codereview.appspot.com/11013043/diff/24001/app/views/view-container.j... app/views/view-container.js:315: @param {Model} to bind to the slot. On 2013/07/09 20:20:22, benjamin.saller wrote: > On 2013/07/09 19:56:23, benji wrote: > > More capitalization and phrasing to check for sane rendering. > > added a word or two, but not sure if we should titlecase formal param names. I'm fine with viewing "viewlet" as the parameter name. I would be inclined to view "viewlet" as a noun separate from the coincidentally named parameter and capitalizing it. https://codereview.appspot.com/11013043/diff/24001/test/test_view_container.js File test/test_view_container.js (right): https://codereview.appspot.com/11013043/diff/24001/test/test_view_container.j... test/test_view_container.js:184: viewContainer.render(); On 2013/07/09 20:20:22, benjamin.saller wrote: > On 2013/07/09 19:56:23, benji wrote: > > I would have an assertion that shows that the viewlet was not rendered before > > .render() was called. > > That is done in the preceding test but I can add it here as well. Oh! I missed that. Either way is fine with me (if it were me I think I would still duplicate the assertion here because it is cheap).
Sign in to reply to this message.
*** Submitted: View slots implementation Viewlet Container can define slots, a mapping of logical slot name to CSS selector within the ViewletContainer DOM. Viewlets can define slot, the logical name within the container to fill. showViewlet takes an optional model when showing a viewlet with a slot. This will remove any old bindings associated with the viewlet and bind in the new model. R= CC= https://codereview.appspot.com/11013043
Sign in to reply to this message.
On 07/09/2013 12:31 PM, benjamin.saller@canonical.com wrote: ... > https://codereview.appspot.com/11013043/diff/7001/app/views/databinding.js#ne... > > app/views/databinding.js:192: var modelEvents = this.unbindModel(model); > On 2013/07/09 01:57:07, gary.poster wrote: >> On first reading, my reaction is to wonder why we unbind the model as > soon as we >> are asked to bind with it, and then to wonder why mutating the result > of the >> call (in the "push" line below) is useful. Maybe it will make more > sense as I >> read more, but at first glance I don't like the way it reads. At the > very least >> this needs some nice comments. Perhaps it also needs some renaming, > or >> refactoring? Hopefully I'll have a more fully-formed opinion after I > read more. > > I now call these methods resetModelChangeEvents and > resetViewletDOMEvents, hope that helps. It is a bit odd perhaps that I > return the list which can be appended to rather than resort to another > lookup in the calling function. If that bothers you I can change it, but > I don't think its that complex given the better naming. I agree that the better naming resolves my concerns. I still prefer the other approach, but perhaps that preference approaches bikeshed painting. > https://codereview.appspot.com/11013043/diff/7001/app/views/databinding.js#ne... > > app/views/databinding.js:225: if (viewlet.rebind) { > On 2013/07/09 01:57:07, gary.poster wrote: >> I still don't like this method name. I think something explicit would > be much >> better, such as getBindModel or getTarget or findModel or > findBindModel or >> findTarget... > > selectBindModel(model) -> model now, better? Yes, better than all of my suggestions, thanks. > https://codereview.appspot.com/11013043/diff/7001/app/views/databinding.js#ne... > > app/views/databinding.js:229: // When rebind doesn't return a valid > target, skip the binding. > On 2013/07/09 01:57:07, gary.poster wrote: >> I guess this is a feature we need. I can't help but wonder if we > ought to write >> something to the console in case this was unintentional, or make using > this >> feature more explicit (catching a specific error, for instance). At > the least, >> if the comment is accurate, the conditional should go within the > previous block; >> and if it is not, the comment should be corrected. > > This occurs in tests where service models might not have a units > lazymodellist. I don't think we want console warning in those cases and > we don't want to have to construct more state to test these things but > if this still bugs you we can refactor some tests down the road. Cool, fine with that. > https://codereview.appspot.com/11013043/diff/7001/app/views/databinding.js#ne... > > app/views/databinding.js:362: var mID = model.get('id'); > On 2013/07/09 01:57:07, gary.poster wrote: >> I worry about our need for POJO support. It's coming in the unit > view, yeah? >> Because that information can change dynamically. And then we have a > bit of >> thinking to do? > >> That's an aside: I don't expect this branch to make the necessary > changes here >> or elsewhere. > > I'd add an identity callback returning the id if/when we actually need > this. Right, with custom id code, I assume. Thanks, Ben. Gary
Sign in to reply to this message.
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
