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

Feature request: Specify transclusion position #300

Closed
johngeorgewright opened this issue Feb 4, 2015 · 95 comments
Closed

Feature request: Specify transclusion position #300

johngeorgewright opened this issue Feb 4, 2015 · 95 comments

Comments

@johngeorgewright
Copy link

It would be great to specify where to include the content of a parent tag. Ie:

<!-- form-group.tag -->
<form-group>
  <div class="form-group">
    <label>{ opts.label }</label>
    { innerHTML } <!-- somehow transclude given content here (there is probably a better name than "innerHTML" ;) -->
  </div>
</form-group>
<!-- index.html -->
<form>
  <form-group label="Field 1">
    <input type="text" name="field-1"/>
  </form-group>
</form>

.... producing ....

<div class="form-group">
  <label>Field 1</label>
  <input type="text" name="field 1"/>
</div>
@ivan-saorin
Copy link
Contributor

It would be useful to me too. +1

@ivan-saorin
Copy link
Contributor

I created a plunk to demonstrate the concept (It may require some refresh to work due to plunker): http://embed.plnkr.co/QtPQCMUCsVDwRz1zV2C3/preview

@johngeorgewright
Copy link
Author

Cool. What about using transclude for the tag name too, just to keep things consistent?

Another option, I don't know if you've thought about it, would be to use a function call, which could be overridden in some situations <div>Other stuff { transclude() }</div>.

@ivan-saorin
Copy link
Contributor

I'd like to have a sort of poll about the name. I know transclusion is the correct technical term: but at the end of the day to it sounds really angularish. I would prefer to keep things understandable by everyone than to be absolutely technically correct. It's only my opinion so I accept alternative proposal for the name (alternative to transclusion).

For the "{ transclude() }" proposal: maybe it could be an alternative method? I like the tag because you are marking the insertion point inside an html template. But why not both. I'll take a look at that.

@ivan-saorin
Copy link
Contributor

Another poll I have inserted two "compiler directives" here. "replacetag" and "transclude" and I took the opportunity to place them inside the opts object as it's already vehicolating correctly through the objects chain.
At a more architectural level It could be better to define that such possibility is offered by the framework in a more specific object. It could be called "api", "behaviour", "component" or other ideas?

@johngeorgewright
Copy link
Author

Sorry, I wasn't implying to change the name of the tag to "transclude" just because I thought it the correct term. I just noticed that you had implemented both an option called "transclude" and a tag called "include" to create the one feature. Keeping them both the same would cause less confusion. And again, once you've decided on what to call the tag, the compiler directive's name will become obvious.

I know what you mean about "transclusion" sounding a bit "angular-esc", however, it is the perfect term as it describes exactly what's happening. "Include" sounds more like pulling an external source in to the document. So "transclude" would be my vote.

As for the object serving this feature, if you're going to be using the behaviour pattern, then "behaviour" is perfect. Otherwise I wouldn't say it would suit. Will it be serving an other features, or just this one?

@teabyii
Copy link

teabyii commented Feb 5, 2015

This feature in "Ember.js", it provides the yield helper, is it a choice?

@johngeorgewright
Copy link
Author

Actually, yeh. "Yield" also makes sense.

@ivan-saorin
Copy link
Contributor

To such an object should be the way to activate framework features from the tag implementation to the framework. It should be general and work for every tag implementation.

@ivan-saorin
Copy link
Contributor

So that for now we have:

Main term Activator Marker
Transclusion transclusion=true transclude
Yeld yeld=true yeld

@ivan-saorin
Copy link
Contributor

I think tag could be the correct term:

with transclude

<alert>
  <div class="alert alert-{category}" role="alert"> {msg} <transclude></div>

  opts.tag.transclusion = true,  // was transclude
  opts.tag.renderRoot = false  // was replacetag

  this.category = opts.category
  this.msg = opts.msg
</alert>

with yeld

<alert>
  <div class="alert alert-{category}" role="alert"> {msg} <yeld></div>

  opts.tag.yeld = true,  // was transclude
  opts.tag.renderRoot = false  // was replacetag

  this.category = opts.category
  this.msg = opts.msg
</alert>

@johngeorgewright
Copy link
Author

Remind me what opts.tag.renderRoot does?

@johngeorgewright
Copy link
Author

yeld => yield

@johngeorgewright
Copy link
Author

I was leaning towards "yield" until I saw it in practice. Now I'm back on liking transclude.

@ivan-saorin
Copy link
Contributor

Ok, for now let's stay stick with transclude. If someone will came up with a good name we will update it later.

opts.tag.renderRoot allows to strip the custom tag from the generated markup.

@johngeorgewright
Copy link
Author

Ah. Cool.

@ivan-saorin
Copy link
Contributor

Ok done with transclusion / transclude.

<alert>
  <div class="alert alert-{category}" role="alert"> {msg} <transclude></div>

  opts.tag.transclusion = true,  // was transclude
  opts.tag.skipRootNode = true  // was replacetag

  this.category = opts.category
  this.msg = opts.msg
</alert>

@tipiirai
Copy link
Contributor

tipiirai commented Feb 9, 2015

I've had following tag on the repository since the beginning:

https://github.com/muut/riotjs/blob/master/test/tag/inner-html.tag

It takes the inner html and appends more stuff to it upon "mount".

I think you can play more with this.root. Let me know how that works

@mschwartz
Copy link

I want to be able to define tags and use them like this:

(from twitter bootstrap http://getbootstrap.com/components/#navbar):

<navbar>
  <left>
    <brand src="logo.gif"></brand>
   <link href="#" active>text 1</link>
   <link href="#" active>text 2</link>
    <dropdown>
      <menuitem href="#">item 1</menuitem>
      <menuitem href="#">item 2</menuitem>
      ...
    </dropdown>
    <search></search>
  </left>
  <right>
    <dropdown>
      <menuitem href="#">item 1</menuitem>
      <menuitem href="#">item 2</menuitem>
      ...
   </dropdown>
  </right>
</navbar>

I don't want to define a navbar tag that uses these other tags, I want a palette of these tags that I can nest as shown in my page. That is, I want to cobble together some number of navbars with many different options for brand, dropdown present or not, etc.

Compare that elegant looking code to the raw HTML:

image

The beauty of such a solution is I can think about the components of the navbar without giving much thought at all to the clumsy HTML elements and styles and ids and so on that need to be generated for the navbar to look as sweet as Bootstrap can make it.

@ivan-saorin
Copy link
Contributor

Totally Agree with that. A sort of Riot-Bootstrap done that way would be wonderful!

@tipiirai
Copy link
Contributor

I think this is best solved with a custom tag. Like this for example:

<some-tag>
  <h1>Hello!</h1>
  <inner-html/>
</some-tag>

Here's the implementation for <inner-html>:

<inner-html>
  var p = this.parent.root
  while (p.firstChild) this.root.appendChild(p.firstChild)
</inner-html>

definitely a useful tag. Maybe we'll have a "core tags" library at some point where this would fit perfectly.

Thoughts?

@johngeorgewright
Copy link
Author

Ah yes, that makes sense. Quite a simple approach.

@mschwartz
Copy link

In the tag layout for navigation I presented, the inner-tags have
this.parent === undefined. I tried it, hence the reason for my comment
here.

On Sat, Feb 14, 2015 at 3:11 AM, Tero Piirainen notifications@github.com
wrote:

I think this is best solved with a custom tag. Like this for example:

Hello!

Here's the implementation for :

var p = this.parent.root while (p.firstChild) this.root.appendChild(p.firstChild)

definitely a useful tag. Maybe we'll have a "core tags" library at some
point where this would fit perfectly.

Thoughts?


Reply to this email directly or view it on GitHub
#300 (comment).

@sheastreet
Copy link

I think the custom tag approach does a good job because It still leaves room for custom transclusion logic. But mschwartz does have a good point and I can see how this can get hairy using custom tag logic if there is not an easy way to get at the riot level data and set the parent manually.

@sheastreet
Copy link

Actually from what I can tell with console logging when using the inner-html custom tag approach is that other nested custom tags look to know their parent before they are moved.

@tipiirai
Copy link
Contributor

@mschwartz can you make a minimalistic jsfiddle where I can see this issue? Thanks!

@johngeorgewright
Copy link
Author

This only works 1 level deep. Any thoughts on how we can nest this feature?

GianlucaGuarini added a commit that referenced this issue Apr 14, 2015
@GianlucaGuarini
Copy link
Member

With my patch you can use the <yield> tag directly in the riotjs core to handle html transclusion.
Usage:
my-tag.tag

<my-tag>
  <p>Hello <yield/></p>
  this.text = 'world'
</my-tag>

Usage

<my-tag>
  <b>{ text }</b>
</my-tag>

Result

<my-tag>
  <p>Hello <b>world</b><p>
</my-tag>

@GianlucaGuarini
Copy link
Member

@johngeorgewright here is one of your jsfiddle using the yield tag http://jsfiddle.net/gianlucaguarini/cf0nqL7m/1/

@johngeorgewright
Copy link
Author

Nice! Been gagging for this feature.

@eyy
Copy link

eyy commented Apr 16, 2015

Great to see it working again. As I said above, I really think the tag content should be parsed in the parent context, as it is in React and others.

@GianlucaGuarini
Copy link
Member

GianlucaGuarini commented Apr 16, 2015

@eyy actually with the yield tag the content is parsed in the parent. I think I will include it in the next riot release

@eyy
Copy link

eyy commented Apr 16, 2015

@GianlucaGuarini but in your comment and fiddle above it is parsed in the child context: http://jsfiddle.net/cf0nqL7m/2/

@GianlucaGuarini
Copy link
Member

@eyy Could you please provide an example in React or vue please? thanks

@eyy
Copy link

eyy commented Apr 16, 2015

@GianlucaGuarini Sure: Vue, React.

@GianlucaGuarini
Copy link
Member

@eyy awesome I will update the yield feature ASAP to behave exactly like this. It makes sense. Thanks for your feedback

@GianlucaGuarini
Copy link
Member

@eyy @johngeorgewright here there's the new jsfiddle with the brand new yield tag http://jsfiddle.net/gianlucaguarini/cf0nqL7m/3/ parsed with the parent attributes like React and Vue do

@johngeorgewright
Copy link
Author

👍

1 similar comment
@ariesjia
Copy link

+1

@valan
Copy link

valan commented Apr 21, 2015

I have a question. I believe something like the below would be possible if yield was evaluated in the child context, but I'm not sure how to go about it or if it would be possible if it's evaluated in the parent context. How would you guys approach it?

<people>
  <person data="{person}"></person>
</people>
<person>
  <div>{person.name} - <strong>{person.phone}</strong></div>
  this.person = opts.data;
</person>

<people>
  <div each="{person in people}">
    <yield/>
  </div>
  this.people = [{name:"bob",phone:"555-1212"},{name:"john",phone:"555-1213"}];
</people>

@GianlucaGuarini
Copy link
Member

@valan You are right at moment it's not possible I just wrote the documentation https://github.com/muut/riotjs/blob/feature/yield/doc/api/tags.md#html-transclusion-using-the-yield-tag--yield

Maybe in future we could think about it

@valan
Copy link

valan commented Apr 21, 2015

so I guess the workaround for that would be putting the template in the tag definition rather than in the page, then maybe make a conditional if a different layout is needed

<people layout="one"></people>
<person>
  <div>{person.name} - <strong>{person.phone}</strong></div>
  this.person = opts.data;
</person>

<people>
  <div if="{layout=='one'}">
    layout 1
    <div each="{person in people}"><person data="{person}"></person></div>
  </div>
  <div if="{layout=='two'}">
    layout 2
    <div each="{person in people}"><person data="{person}"></person></div>
  </div>
  this.people = [{name:"bob",phone:"555-1212"},{name:"john",phone:"555-1213"}];
  this.layout = opts.layout;
</people>

@GianlucaGuarini
Copy link
Member

Now that the yield tag will land in the new riot release #617 I think I will remove the compiling at the parent level like suggested from @eyy and like Vue and React do.
The reasons I want remove this behavior are these:

  • having two different ways of compiling the yield tag makes it de facto inconsistent and it could generate other confusion to our users
  • the code needed to support it is much more complex
  • by compiling the injected html using always its tag context, the yield tag in a child could always have access to the parent data but this is not possible the other way around if we compile it using the parent attributes ( see Feature request: Specify transclusion position #300 (comment) )

@valan
Copy link

valan commented Apr 22, 2015

Yeah, child context makes way more sense intuitively, at least to me. And you can always access the parent if you need to anyway.

@eyy
Copy link

eyy commented Apr 23, 2015

Well, I'll give one last advocation for parent-context compiling:

  • Only one way to compile yield: I can't see why should there be two different ways of yield compiling. Code should be understood only in its context: when we pass arguments to a function (var a = 1, b = 2; func(a + b)) they are evaluated before the function runs. This is the same case here.
  • Maybe it's not that complex: I should probably play first with the code, and anyhow the link @GianlucaGuarini gave above as an example of how complex it is isn't working, but I don't think the the implementation should be much more complex. Maybe yield shouldn't even be a tag, but a property: <div>{ opts.yield }</div>, saved to a string in the parent-context and only displayed within the child.
  • Encapsulation: What's more, the basic rule of encapsulation is broken. Riot basic promise is to keep the template and logic next to each other. But in @valan's code above, e.g., someone looking at <person data="{person}"></person> must guess where this person is coming form. And should the <person> tag change its internal naming or structure, other modules might fail loudly, or even worse, silently.
  • HTML extension, not restructure: I gave an example at my first comment here: changing <h2>{ name }</h2> to <big-title>{ name }</big-title> shouldn't break anything.
  • Top-down flow: Children receive their data from above, therefore the parent has natural access to the child (this.people in @valan's example contains each person), while the opposite depends the troublesome magical this.parent.
  • Community standard: If React, Vue and the Web Component spec follow this behaviour, I think that it speaks for itself, better than I did.

I'll finish with my real-world example: I wanted a popup tag, to provide different components with a simple close-button, open() and close() methods, and a blacked out background screen. Here is what I should have probably wrote:

<pop>
  <div class="black-screen hide"></div>
  <div class="popup hide">
    <a href="#" onclick={ close }>X</a>
    <yield />
  </div>
  <script>
    let tags = $('.black-screen, .popup', this.root)
    this.close = () => tags.addClass('hide')
    this.show = () => tags.removeClass('hide')
  </script>
</pop>

<app>
  <pop name="won"><h2>You Won! Congrats, { name }</h2></pop>
  <button onclick={ tags.won.show }>Roll?</button>
  <script>
    this.name = 'John'
  </script>
</app>

If that hasn't convinced, well, it's ok. Maybe if i'll have time i'll mess with the code a bit.
Thanks.

@GianlucaGuarini
Copy link
Member

@eyy thanks for your complete answer. The thing is much more complicate than what you think. What happens to the content of the nested loops? .. And of the nested loops of other internal loops? If we start handling all this cases we will end up writing angularjs. We need to keep riot dry using the yield tag we can cover the most of transclusion cases in a clear and simple way, without expecting any surprise. I'am going to close this issue for now. And I would be really happy to dig it more this argument in another issue. Thanks to for all your useful examples you helped me a lot ✌️

@eyy
Copy link

eyy commented Apr 25, 2015

✌️

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

No branches or pull requests