Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for parameters on inline partials/blocks #358

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

mattpowell
Copy link

This adds support for passing parameters/attributes to inline partials/blocks. This change will cause inline partials to act almost exactly the same passing parameterss to a section or (non-inline) partial (wrt setting and passing parameters).

I've added tests for three use-cases, but am happy to add to or remove them if you think a different test would be more expressive of this change. Also, note, all tests are passing locally.

Below are a few more examples of how parameters on inline partials will work after this PR:

{! JSON: {"search_url": "http://google.com/search"} !}
{<partial}
Partial url is: {url}
{/partial}
{+partial url=search_url/}

Outputs: Partial url is: http://google.com/search

{<partial}
Partial url is: {url}
{/partial}
{+partial url="http://google.com/search"/}

Outputs: Partial url is: http://google.com/search

{! JSON: {"url": "http://google.com/search"} !}
{<partial}
Partial url is: {url}
{/partial}
{+partial/}

Outputs: Partial url is: http://google.com/search

{<partial}
Partial url is: {url}
{/partial}
{+partial url=nonExistentRef/}

Outputs: Partial url is:

{! JSON: {"search_url": { "value: "http://google.com/search" } } !}
{<partial}
Partial url is: {url.value}
{/partial}
{+partial url=search_url/}

Outputs: Partial url is: http://google.com/search

{! JSON: {"url": "http://google.com/search" } !}
{<partial}
Partial url is: {url}
{/partial}
{+partial url=search_url/}

Outputs: Partial url is: http://google.com/search

{! JSON: {"url": "http://google.com/search" } !}
{<partial}
Partial url is: {.url}
{/partial}
{+partial url=search_url/}

Outputs: Partial url is:

Thanks and let me know if I need to update any READMEs or wikis anywhere.

@prashn64
Copy link
Contributor

Can you give a real use case, because the way I use blocks is one base template which contains the blocks and sub templates that include that base template and override those blocks such as:

base.tl

{<content
   Here is my content {content}
/}

article.tl

{>"base" content="awesome content"/}

{+content/}

So basically my question is if the above solves all use cases, or if there is a use case where you can't just pass parameters to a base template(since there may not be one for ex).

'{+formAction url=refDoesntExist/}'].join("\n"),
context: {},
expected: '<form action=""></form>',
message: "should test blocks with inline parameters"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add 2 unit tests.

One where the ref doesn't exist in the context, but you pass a static string for the param value.

The second where you reference include another template that has the blocks, and then you call that block in your source. You can include previously defined tests as partials by their name.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ran in to a little snag w/ the second unit test, but, updated w/ something that I think get's close to what you're asking for... mind taking a look and letting me know?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So the problem may be how blocks and inline partials work today. @jimmyhchan and I were playing around with them, and they really just made no sense. However, I think your original PR is useful, so let's stick with that, and then revisit this issue more deeply later on.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds like a plan. I'll revert the partial_with_blocks_and_overridable_params stuff, but, keep the blocks with a static string as an inline parameter unit test since I think it still makes sense...

…n inline parameter to an inline partial and another that tests defining static params on a block.
@prashn64
Copy link
Contributor

lgtm, let's get another pair of eyes on it.

@smfoote
Copy link
Contributor

smfoote commented Nov 20, 2013

Looks like you need to git pull before we try to merge.

@smfoote
Copy link
Contributor

smfoote commented Nov 20, 2013

Does this work with two separate partials?

{! base.tl !}
{+partial url=search_url/}
{! override.tl !}
{>"base.tl"/}
{<partial}
  Partial url is: {url}
{/partial}

My concern is that the blocks are added as global(ish) variables at the partial level. Since these are different partials, will the params be added to the correct context?

Also, what happens if the inline partial is within a different context than the block?

{+partial url=search_url/}

{#myOtherContext}
  {<partial}
    Partial url is: {url}
  {/partial}
{/myOtherContext}

I'm guessing that this won't work.

@jimmyhchan
Copy link
Contributor

See #313 This will be inconsistent with how params are treated in sections and partials when there is a conflict.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants