Bug 996526 - Backbone views to carry their own templates. #20
Conversation
| */ | ||
| var BaseView = Backbone.View.extend({ | ||
| var L10nView = (function() { |
There was a problem hiding this comment.
I like this solution, I think it might lead to a bit of over-translation, but as we'll most likely need to watch performance anyway, I think we'll catch it if it gets too bad.
There was a problem hiding this comment.
This a supremely great hack. That said, monkeypatching Backbone.View.extend() so that it can, in turn, monkeypatch every render method feels a little scary to me. Perhaps mostly because it feels like it causes render to do something slightly different and non-intuitive compared to what it does in normal views. This might be eased if L10Views were expected to implement something like localizedRender instead of render.
Or maybe I should just get over my discomfort and embrace the hack. :-)
Food for thought; your call on what you want to do here, if anything.
There was a problem hiding this comment.
I really felt like keeping the Backbone convention for rendering the views… The idea is that you can switch between L10nViews and regular ones while keeping the same API usage, which is great if you ask me.
| @@ -41,7 +41,13 @@ loop.webapp = (function($, OT, webl10n) { | |||
| * as a `model` property. | |||
| */ | |||
| var ConversationFormView = sharedViews.BaseView.extend({ | |||
| el: "#conversation-form", | |||
| template: _.template([ | |||
| '<form>', | |||
There was a problem hiding this comment.
The original HTML had an autocomplete="off" attribute. Any particular reason that's disappeared here?
There was a problem hiding this comment.
Because it's no more necessary as the view is destroyed, created and rendered live instead of relying on the previous existing DOM state.
|
In general, this looks great. One other comment: running the client-local tests now generates 9 "is undefined" warnings from [l10n] in the web console, whereas without the patch, there were just 5. I get that these are just warnings, but they do make it harder to locate the console messages that are actually worth acting in. If possible, it'd be great to get rid of these warnings. If it's more practical, just not regressing the number would be a reasonable start. If you tell me it's more pain than it's worth, I can live with that too. r=dmose, with the comments addressed. As usual, please squash and add the r= notation to the commit message before merging. |
We still need to find a way to mock l10n in shared tests. That should be addressed in a dedicated PR though. For now, most nits will be addressed in #22 to avoid rebasing and resolving too many conflicts for no good reason :) Merging. |
Bug 996526 - Backbone views to carry their own templates. r=dmose
Related bug: https://bugzilla.mozilla.org/show_bug.cgi?id=996526