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

Pass custom element attributes to Glimmer component element #15

Merged
merged 1 commit into from
Apr 19, 2017

Conversation

pittst3r
Copy link
Contributor

@pittst3r pittst3r commented Apr 15, 2017

Requires glimmerjs/glimmer-application#40. Intermediate step until we can have args and attrs passed into renderComponent.

Given a template like this:

{{! cool-component.hbs }}
<div>
  <p>Hello Glimmer!</p>
</div>

And a custom element like this:

<cool-component foo="bar"></cool-component>

We get this as the result:

<div foo="bar">
  <p>Hello Glimmer!</p>
</div>

@pittst3r pittst3r requested a review from tomdale April 15, 2017 13:07
@pittst3r
Copy link
Contributor Author

Found a bug, needs a bit more work.

@pittst3r pittst3r removed the request for review from tomdale April 15, 2017 16:21
@pittst3r pittst3r changed the title Pass custom element attributes to Glimmer component element [WIP] Pass custom element attributes to Glimmer component element Apr 15, 2017
@pittst3r pittst3r changed the title [WIP] Pass custom element attributes to Glimmer component element Pass custom element attributes to Glimmer component element Apr 15, 2017
@pittst3r
Copy link
Contributor Author

K, this seems ready to roll now.

@rwjblue
Copy link
Member

rwjblue commented Apr 15, 2017

I was thinking that we would actually roll this into the main "pass attributes and args into app.renderComponent" story. IMO, we actually want these attributes to be set prior to the custom element is added to the DOM (otherwise we are not really following the "normal" DOM construction spec).

When we can pass attributes and arguments in to Application#renderComponent, this becomes fairly similar to what you have now, but we would grab the attributes first and pass them in:

let attributes = getAttributes(this);
app.renderComponent(name, parent, placeholder, attributes, /* args? */)

// ...snip...

function getAttributes(fromElement: Element): Dict<string> {
  let output = Object.create(null);
  let attributes = fromElement.attributes;
 
  for (let i = 0; i < attributes.length; i++) {
    let { name, value } = attributes.item(i);
    output[name] = value;
  }
  
  return output;
}

@pittst3r
Copy link
Contributor Author

Just a recap of our Slack convo...

@rwjblue 100% agreed. It's definitely part of the plan to get renderComponent to accept attributes and args. I just failed to communicate that this is an intermediate step since that is not a hard blocker.

@rwjblue
Copy link
Member

rwjblue commented Apr 15, 2017

👍 - Awesome, thanks for walking me through it!

@tomdale
Copy link
Contributor

tomdale commented Apr 19, 2017

Seems hacky but this is a glaring omission right now. I'll be very happy once renderComponent accepts attributes. 😉

@tomdale tomdale merged commit 9d7133e into master Apr 19, 2017
@pittst3r pittst3r deleted the attributes branch April 19, 2017 16:49
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.

3 participants