From f75ab89eb2515f7e08fc87ecdf8f02bd5110c315 Mon Sep 17 00:00:00 2001 From: Tom Coleman Date: Fri, 11 Apr 2014 15:26:04 +1000 Subject: [PATCH 01/13] Basic interface working as in Verso. For #11 --- layout.js | 21 ++++++++++++++++++--- overrides.js | 2 +- 2 files changed, 19 insertions(+), 4 deletions(-) diff --git a/layout.js b/layout.js index 454f7c0..1af8838 100644 --- a/layout.js +++ b/layout.js @@ -108,7 +108,12 @@ Layout = UI.Component.extend({ var dataDep = new Deps.Dependency; var regions = this._regions = new ReactiveDict; var content = this.__content; - + + // look first in regions that have been explicitly set, then data + var getRegion = function(region) { + return self._regions.get(region) || self.get(region); + } + // a place to put content defined like this: // {{#contentFor region="footer"}}content{{/contentFor}} // this will be searched in the lookup chain. @@ -200,6 +205,8 @@ Layout = UI.Component.extend({ region = 'main'; self.region = region; + console.log('data', data) + self.text = !! data.text; // reset the data function to use the layout's // data @@ -217,10 +224,14 @@ Layout = UI.Component.extend({ // changes, this comp will be rerun and the new template // will get put on the screen. return function () { - var regions = layout._regions; // create a reactive dep - var tmpl = regions.get(region); + var tmpl = getRegion(region); + console.log(self.text) + if (self.text) + return tmpl; + + console.log('looking for', tmpl) if (tmpl) return lookupTemplate.call(layout, tmpl); else if (region === 'main' && content) { @@ -231,6 +242,10 @@ Layout = UI.Component.extend({ }; } }); + + this.hasYield = function(region) { + return !! getRegion(region); + }; // render content into a yield region using markup. when you call setRegion // manually, you specify a string, not a content block. And the diff --git a/overrides.js b/overrides.js index 319bcb8..6e26e87 100644 --- a/overrides.js +++ b/overrides.js @@ -53,7 +53,7 @@ UI.Component.lookup = function (id, opts) { if (id === 'yield') { throw new Error("Sorry, would you mind using {{> yield}} instead of {{yield}}? It helps the Blaze engine."); - } else if (id === 'contentFor') { + } else if (id === 'contentFor' || id === 'hasYield') { var layout = findComponentOfKind('Layout', this); if (!layout) throw new Error("Couldn't find a Layout component in the rendered component tree"); From d5f7b052603ea8c7c77ae658790b0eafcf024def Mon Sep 17 00:00:00 2001 From: Tom Coleman Date: Fri, 11 Apr 2014 16:18:54 +1000 Subject: [PATCH 02/13] Embox. --- layout.js | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/layout.js b/layout.js index 1af8838..451d2d8 100644 --- a/layout.js +++ b/layout.js @@ -111,7 +111,10 @@ Layout = UI.Component.extend({ // look first in regions that have been explicitly set, then data var getRegion = function(region) { - return self._regions.get(region) || self.get(region); + // Embox it so yields don't re-render when it doesn't change. + return UI.emboxValue(function() { + return self._regions.get(region) || self.get(region); + })(); } // a place to put content defined like this: @@ -205,7 +208,6 @@ Layout = UI.Component.extend({ region = 'main'; self.region = region; - console.log('data', data) self.text = !! data.text; // reset the data function to use the layout's @@ -227,11 +229,9 @@ Layout = UI.Component.extend({ // create a reactive dep var tmpl = getRegion(region); - console.log(self.text) if (self.text) return tmpl; - console.log('looking for', tmpl) if (tmpl) return lookupTemplate.call(layout, tmpl); else if (region === 'main' && content) { From 6ccf219264b27ccf191564f9d1bb94873f8157ae Mon Sep 17 00:00:00 2001 From: Tom Coleman Date: Tue, 15 Apr 2014 18:25:16 +1000 Subject: [PATCH 03/13] Data can be null --- layout.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/layout.js b/layout.js index 451d2d8..b7ad18e 100644 --- a/layout.js +++ b/layout.js @@ -208,7 +208,7 @@ Layout = UI.Component.extend({ region = 'main'; self.region = region; - self.text = !! data.text; + self.text = !! (data && data.text); // reset the data function to use the layout's // data From 812320c24483db0bdbf453b6a2695e176355c742 Mon Sep 17 00:00:00 2001 From: Tom Coleman Date: Thu, 1 May 2014 17:38:10 +1000 Subject: [PATCH 04/13] Don't set `this.data`, rather use `UI.with`. This means that the component, or specifically `getRegion()` will receive updates to data reactively (for instance if you do `{{#Layout template='layoutWithFooter' footer=someHelperThatReturnsATemplateName}}`). So we can use `data()` as a secondary source of region names reliably, and all is roses :) --- layout.js | 69 +++++++++++++++++++++++++++---------------------------- 1 file changed, 34 insertions(+), 35 deletions(-) diff --git a/layout.js b/layout.js index b7ad18e..117068a 100644 --- a/layout.js +++ b/layout.js @@ -160,10 +160,6 @@ Layout = UI.Component.extend({ return val; }; - this.data = function () { - return self.getData(); - }; - /** * Set a region template. * @@ -228,6 +224,7 @@ Layout = UI.Component.extend({ return function () { // create a reactive dep var tmpl = getRegion(region); + log('rendering yield', region, tmpl) if (self.text) return tmpl; @@ -304,38 +301,40 @@ Layout = UI.Component.extend({ render: function () { var self = this; - // return a function to create a reactive - // computation. so if the template changes - // the layout is re-endered. - return function () { - // reactive - var tmplName = self.template(); - - //XXX hack to make work with null/false values. - //see this.template = in ctor function. - if (tmplName === '_defaultLayout') - return self._defaultLayout; - else if (tmplName) { - var tmpl = lookupTemplate.call(self, tmplName); - // it's a component - if (typeof tmpl.instantiate === 'function') - // See how __pasthrough is used in overrides.js - // findComponentWithHelper. If __passthrough is true - // then we'll continue past this component in looking - // up a helper method. This allows this use case: - // - tmpl.__passthrough = true; - return tmpl; - } - else { - return self['yield']; + return UI.With(_.bind(self.getData, self), UI.block(function () { + // return a function to create a reactive + // computation. so if the template changes + // the layout is re-endered. + return function() { + // reactive + var tmplName = self.template(); + + //XXX hack to make work with null/false values. + //see this.template = in ctor function. + if (tmplName === '_defaultLayout') + return self._defaultLayout; + else if (tmplName) { + var tmpl = lookupTemplate.call(self, tmplName); + // it's a component + if (typeof tmpl.instantiate === 'function') + // See how __pasthrough is used in overrides.js + // findComponentWithHelper. If __passthrough is true + // then we'll continue past this component in looking + // up a helper method. This allows this use case: + // + tmpl.__passthrough = true; + return tmpl; + } + else { + return self['yield']; + } } - }; + })); } }); From d3289376c0cb5c667630c268500f01b800de07c7 Mon Sep 17 00:00:00 2001 From: Tom Coleman Date: Thu, 1 May 2014 17:43:09 +1000 Subject: [PATCH 05/13] Added tests to wrap it all up --- layout-test.html | 12 ++++++++++++ layout-test.js | 20 ++++++++++++++++++++ 2 files changed, 32 insertions(+) diff --git a/layout-test.html b/layout-test.html index c9f3f26..453fb54 100644 --- a/layout-test.html +++ b/layout-test.html @@ -54,3 +54,15 @@ footer {{/contentFor}} + + + + \ No newline at end of file diff --git a/layout-test.js b/layout-test.js index 24daac8..29a703f 100644 --- a/layout-test.js +++ b/layout-test.js @@ -222,3 +222,23 @@ Tinytest.add('layout - region templates not found in lookup', function (test) { document.body.removeChild(div); } }); + + +Tinytest.add('layout - set regions via arguments - static', function (test) { + withRenderedLayout({template: 'RegionArgumentsTestStatic'}, function (layout, screen) { + test.equal(screen.innerHTML.compact(), 'insideone', 'One template should render into footer region'); + }); +}); + +Tinytest.add('layout - set regions via arguments - dynamic', function (test) { + var footerTemplate = new ReactiveVar('One'); + Template.RegionArgumentsTestDynamic.footerHelper = function() { return footerTemplate.get(); } + + withRenderedLayout({template: 'RegionArgumentsTestDynamic'}, function (layout, screen) { + test.equal(screen.innerHTML.compact(), 'insideone', 'One template should render into footer region'); + + footerTemplate.set('Two'); + Deps.flush() + test.equal(screen.innerHTML.compact(), 'insidetwo', 'Two template should render into footer region'); + }); +}); \ No newline at end of file From 8dc045b53b60a0b867d0cabad36f5aada9b514dd Mon Sep 17 00:00:00 2001 From: Tom Coleman Date: Thu, 1 May 2014 18:12:19 +1000 Subject: [PATCH 06/13] Use `Deps.cache` --- layout.js | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/layout.js b/layout.js index 117068a..18ec54c 100644 --- a/layout.js +++ b/layout.js @@ -110,11 +110,13 @@ Layout = UI.Component.extend({ var content = this.__content; // look first in regions that have been explicitly set, then data + var regionCaches = {}; var getRegion = function(region) { - // Embox it so yields don't re-render when it doesn't change. - return UI.emboxValue(function() { + regionCaches[region] = regionCaches[region] || Deps.cache(function () { return self._regions.get(region) || self.get(region); - })(); + }); + + return regionCaches[region].get() } // a place to put content defined like this: From 0d7cfaa3ad602041e0bc92a0660ce976e9a0caa3 Mon Sep 17 00:00:00 2001 From: Tom Coleman Date: Fri, 2 May 2014 17:01:03 +1000 Subject: [PATCH 07/13] Added test for IR #276 @cmather -- notice that it fails against master. Good job! --- layout-test.html | 4 ++++ layout-test.js | 22 ++++++++++++++++++++++ 2 files changed, 26 insertions(+) diff --git a/layout-test.html b/layout-test.html index c9f3f26..3d7f2c8 100644 --- a/layout-test.html +++ b/layout-test.html @@ -54,3 +54,7 @@ footer {{/contentFor}} + + diff --git a/layout-test.js b/layout-test.js index 24daac8..f6b6b97 100644 --- a/layout-test.js +++ b/layout-test.js @@ -222,3 +222,25 @@ Tinytest.add('layout - region templates not found in lookup', function (test) { document.body.removeChild(div); } }); + + + +// SEE IR#276 for detailed discussion +Tinytest.add('layout - Templates render with correct data even if setData is called after setRegion', function (test) { + withRenderedLayout({template: 'LayoutWithOneYield'}, function (layout, screen) { + Template.TemplateWithHelper = function() {}; + layout.setData(false); + layout.setRegion('One'); + Deps.flush(); + test.equal(screen.innerHTML.compact(), 'layoutone'); + + Template.TemplateWithCreatedCallback.created = function() { + test.equal(this.data, true); + } + + layout.setRegion('TemplateWithCreatedCallback'); + layout.setData(true); + Deps.flush(); + test.equal(screen.innerHTML.compact(), 'layoutcallback'); + }); +}); \ No newline at end of file From cf456eb2ea4ee6383df85cc3d1a89fe5d8e9c97e Mon Sep 17 00:00:00 2001 From: Tom Coleman Date: Tue, 8 Apr 2014 21:31:26 +1000 Subject: [PATCH 08/13] Added failing test for using `{{#with` in layouts Conflicts: layout-test.js --- layout-test.html | 6 ++++++ layout-test.js | 15 ++++++++++++++- 2 files changed, 20 insertions(+), 1 deletion(-) diff --git a/layout-test.html b/layout-test.html index 3d7f2c8..59d98ed 100644 --- a/layout-test.html +++ b/layout-test.html @@ -26,6 +26,12 @@ {{> yield region="footer"}} + + diff --git a/layout-test.js b/layout-test.js index f6b6b97..7ed13a7 100644 --- a/layout-test.js +++ b/layout-test.js @@ -224,7 +224,6 @@ Tinytest.add('layout - region templates not found in lookup', function (test) { }); - // SEE IR#276 for detailed discussion Tinytest.add('layout - Templates render with correct data even if setData is called after setRegion', function (test) { withRenderedLayout({template: 'LayoutWithOneYield'}, function (layout, screen) { @@ -242,5 +241,19 @@ Tinytest.add('layout - Templates render with correct data even if setData is cal layout.setData(true); Deps.flush(); test.equal(screen.innerHTML.compact(), 'layoutcallback'); + +Tinytest.add('layout - set data via with', function (test) { + withRenderedLayout({template: 'LayoutThatSetsData'}, function (layout, screen) { + layout.setRegion('main', 'ChildWithData'); + + layout.setData({ + title: 'parentTitle', + childData: { + title: 'childTitle' + } + }); + + Deps.flush(); + test.equal(screen.innerHTML.compact(), 'childchildTitle'); }); }); \ No newline at end of file From 6df604686844be064fd1c8e2f94a513e9b50d89c Mon Sep 17 00:00:00 2001 From: Tom Coleman Date: Tue, 8 Apr 2014 22:31:44 +1000 Subject: [PATCH 09/13] Made the test a little better --- layout-test.html | 3 +++ layout-test.js | 5 +++-- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/layout-test.html b/layout-test.html index 59d98ed..4cf41b5 100644 --- a/layout-test.html +++ b/layout-test.html @@ -30,6 +30,9 @@ {{#with childData}} {{> yield}} {{/with}} + {{#with childData}} + {{> yield region="footer"}} + {{/with}}