Issue #8 Support cleaner way to pass arguments to partials #13

Merged
merged 5 commits into from May 14, 2012

Projects

None yet

3 participants

@jimmyhchan
Member

Please see the pull request. Adding parameters was pretty straightforward but it's odd what context to give it.

Currently only sections create new contexts. When you add parameters in sections, Dust puts the parameters context on top, followed by the new context. That way, the current context inside a section becomes the new context and the parameters context become the first head of the tail.

I'm mimicking this behavior in the partials parameters. To keep it so that partials do not create a new context (that is, current context should not change but parameters need to be available) I push the parameter context between the current head and the first head of the tail.

@rragan
Collaborator
rragan commented Apr 25, 2012

Wouldn't it be more regular with the rest of the language constructs to have the partial method take chunk, context, blocks, params since you are adding params? It seems like there could well be useful partials that could consume blocks a part of what they do.

@vybs
vybs commented Apr 25, 2012

Woot! Thanks a lot Jimmy!

Some more test cases I may have in mind, will reply to this soon!

@vybs
vybs commented Apr 25, 2012

@rragan, a partial within itself can have any conten, block etc,

Can you elaborate with an example use case?

@rragan
Collaborator
rragan commented Apr 25, 2012

We have a custom JSP tag that I would conisder as a partial with a block. It is call cdiv for "conditionaL DIV". If it has no body text it generates nothing. If it has a body it wraps it in the div. We use this in layouts where the user may or may not supply parts of the layouts.

@vybs
vybs commented Apr 25, 2012

We decided to use @ tags in such scenarios. We do have similar cases, @ module

   /** 
    @author Hans Brough 
    * Used to standardize HTML containers. 
    * @method module
    * @param {Object} params a configuration object created from attributes set in the template - see below for details. 
   */
  dust.helpers.module = function(chunk, context, bodies, params){
    var hasHdr,hdrTag,id,modClass,modType,title;
     if( params ){
        // init
         hasHdr = (params.hasHdr == undefined || params.hasHdr.toLowerCase() === 'true');
         .....
        //starting container tags
        chunk.write("<div class='module mod-" + modType + modClass +"' id='" + id + "'>");
        if( hasHdr ){
          chunk.write("<div class='header'><" + hdrTag + ">" + title + "</" + hdrTag + "></div>");
        }
         chunk.write("<div class='content'>");

          // body itself can be a partial
         chunk.render( bodies.block, context);

         // closing containe tags
        chunk.write("</div></div>");
      }
     return chunk;
   };
@vybs
vybs commented May 9, 2012

@jimmyhchan , i dont see a option to merge this in?

@jimmyhchan
Member

There's a pull request here. #13

On Tue, May 8, 2012 at 10:42 PM, Veena Basavaraj <
reply@reply.github.com

wrote:

@jimmyhchan , i dont see a option to merge this in?


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

@vybs
vybs commented May 9, 2012

It says this for me:

This pull request cannot be automatically merged.

@jimmyhchan
Member

I need to get my stuff synced with the current head... let me get that fixed.

@vybs
vybs commented May 9, 2012

No rush.

We found a good use case to use this feature.

The fs embed will will be partial and take parameters.

fs_embed url="{uri}" template="{template}"

So i'd like to get this in via git svn this week.

@jimmyhchan
Member

@vybs I've updated my fork. The pull request is now clean.

@vybs vybs commented on the diff May 14, 2012
test/jasmine-test/spec/examples.js
@@ -301,6 +301,19 @@ var dustExamples = [
message: "should test partial context"
},
{
+ name: "partial_param",
+ source: '{>replace name=n count="{c}"/}',
+ context: { n: "Mick", c: 30 },
+ expected: "Hello Mick! You have 30 new messages.",
+ message: "should test partial params"
+ },
+ {name: "partial_array_param",
+ source: '{#n}{>replace name=. count="30"/}{@sep} {/sep}{/n}',
+ context: { n: ["Mick", "Tom", "Bob"] },
+ expected: "Hello Mick! You have 30 new messages. Hello Tom! You have 30 new messages. Hello Bob! You have 30 new messages.",
+ message: "should test partial params using an array"
+ },
@vybs
vybs May 14, 2012

lgtm.

Few more unit tests

  1. if there is context override on the partial

partial:ctx name="joe"

and ctx also has name ( the expected behaviour ?) can we add a unit test for this.

@vybs vybs commented on the diff May 14, 2012
test/jasmine-test/spec/examples.js
@@ -301,6 +301,19 @@ var dustExamples = [
message: "should test partial context"
},
{
+ name: "partial_param",
@vybs
vybs May 14, 2012

partial:ctx

also please add unit tests to support name=name ( which means it will not walk up the tree and look for name in the ctx only )

@vybs vybs merged commit a82d9f8 into linkedin:master May 14, 2012
@vybs vybs commented on the diff Jun 29, 2012
test/jasmine-test/spec/examples.js
@@ -301,6 +301,19 @@ var dustExamples = [
message: "should test partial context"
},
{
+ name: "partial_param",
+ source: '{>replace name=n count="{c}"/}',
+ context: { n: "Mick", c: 30 },
+ expected: "Hello Mick! You have 30 new messages.",
+ message: "should test partial params"
+ },
+ {name: "partial_array_param",
+ source: '{#n}{>replace name=. count="30"/}{@sep} {/sep}{/n}',
@vybs
vybs Jun 29, 2012

@jimmyhchan

i forget this

{>"tl/apps/demo/embed/base_embed" test="FOOOO"/}
{>"tl/apps/demo/embed/base_embed" test="BARRR"/}

{>"tl/apps/demo/embed/base_embed" test=foo/}
{>"tl/apps/demo/embed/base_embed" test=bar/}

the above output for test is always the value for "bar" in the JSON

is this expected?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment