Skip to content

Allow body attributes#171

Merged
ef4 merged 1 commit intoember-fastboot:masterfrom
ef4:body-attributes
Oct 26, 2017
Merged

Allow body attributes#171
ef4 merged 1 commit intoember-fastboot:masterfrom
ef4:body-attributes

Conversation

@ef4
Copy link
Contributor

@ef4 ef4 commented Oct 26, 2017

The SimpleDOM document that gets created during fastboot rendering has a body tag, and it's possible to set attributes on it. For example, assuming getDOM from ember-wormhole:

Component.extend({
  init() {
    this._super(..arguments);
    getDOM(this).body.setAttribute('data-foo', 'hello')
  }
})

But this doesn't actually cause any output in fastboot, because the user's own <body> tag from index.html is always used instead.

This PR allows attributes set on the simpledom body to be copied into the output.

Given that the user's index.html is treated as a string and handled with string substitution, I'm forced to do a Regexp-based replaced that is less elegant than we would be able to do if we parsed the index.html file. But that seems like a pretty big change that would need more thought. So I think the string substitute is good enough here.

add test

The SimpleDOM document that gets created during fastboot rendering has a body tag, and it's possible to set attributes on it. For example, assuming `getDOM` from [ember-wormhole](https://github.com/yapplabs/ember-wormhole/blob/6992bb2a9fced9183cdb426a931722fb1fefba8a/addon/utils/dom.js#L45-L63):

```js
Component.extend({
  init() {
    this._super(..arguments);
    getDOM(this).body.setAttribute('data-foo', 'hello')
  }
})
```

But this doesn't actually cause any output in fastboot, because the user's own `<body>` tag from index.html is always used instead.

This PR allows attributes set on the simpledom body to be copied into the output.

Given that the user's `index.html` is treated as a string and handled with string substitution, I'm forced to do a Regexp-based replaced that is less elegant than we would be able to do if we parsed the `index.html` file. But that seems like a pretty big change that would need more thought. So I think the string substitute is good enough here.

add test
Copy link
Contributor

@kratiahuja kratiahuja left a comment

Choose a reason for hiding this comment

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

LGTM.

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