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

Update Assets.php #1687

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

Update Assets.php #1687

wants to merge 3 commits into from

Conversation

paulmassen
Copy link
Contributor

This is a small addition to the previous change.
This allows to have multiple type on inlined js. Without this change, if you have something like

{% do assets.addInlineJs('Hello', 100,'', "somespecialtype") %}
{% do assets.addInlineJs('Good Morning', 100,'', "anothertype") %}
{% do assets.addInlineJs('Goodbye', 100,'') %} // regular inlined js
{% do assets.addInlineJs('Bonjour', 100,'') %} // regular inlined js

It will be rendered like this:

<script type="somespecialtype">
Hello 
Good Morning
Goodbye
Bonjour
</script>

With this fix, it should display

<script type="anothertype">
Good Morning
</script>

<script type="somespecialtype">
Hello
</script>

// These two inlined js has no type specified, so we can group them on the same tag
<script>
Bonjour 
Goodbye
</script>

This is a small addition to the previous change. 
This allows to have multiple type on inlined js. Without this change, if you have something like
```
{% do assets.addInlineJs('Hello', 100,'', "somespecialtype") %}
{% do assets.addInlineJs('Good Morning', 100,'', "anothertype") %}
{% do assets.addInlineJs('Goodbye', 100,'') %} // regular inlined js
{% do assets.addInlineJs('Bonjour', 100,'') %} // regular inlined js
```

It will be rendered like this:

```
<script type="somespecialtype">
Hello 
Good Morning
Goodbye
Bonjour
</script>
```

With this fix, it should display 
```
<script type="anothertype">
Good Morning
</script>

<script type="somespecialtype">
Hello
</script>

// These two inlined js has no type specified, so we can group them on the same tag
<script>
Bonjour 
Goodbye
</script>
```
Copy link
Member

@mahagr mahagr left a comment

Choose a reason for hiding this comment

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

It looks like the change changes the order in which the scripts appear. It could be a bad thing, though I cannot find any examples right now.

@@ -663,14 +663,18 @@ public function js($group = 'head', $attributes = [])

// Render Inline JS
foreach ($this->inline_js as $inline) {
if ($group && $inline['group'] == $group) {
if (($group && $inline['group'] == $group) && ($inline['type'] == '')) {
Copy link
Member

Choose a reason for hiding this comment

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

Should be rewritten as:

if ($group && $inline['group'] === $group && $inline['type'] === '') {

as per recommendation
@paulmassen
Copy link
Contributor Author

Requested changes applied

@mahagr
Copy link
Member

mahagr commented Oct 16, 2017

What about the ordering? I'm a bit uneasy as the assets are not added in the order they were added by the code.

@paulmassen
Copy link
Contributor Author

Indeed.
A solution would be to add type to all inlined js. As per this commit: 5cd745f

The cost is that if several inlined js is added, it will output multiple script tags but it is valid.

What do you think?

@mahagr
Copy link
Member

mahagr commented Oct 16, 2017

Will need to discuss this with @rhukster . Multiple tags has the benefit that broken js doesn't break rest of the js, but on the other hand single takes less bytes. Also the default type should be removed in html5.

@paulmassen
Copy link
Contributor Author

@mahagr I took another look at this, and it seems like the ordering is still correct, it was just wrong in my manually typed example.
There is still the problem of the defaut text/javascript displayed when no type is set, but I'm not sure I will be able to solve that.

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.

2 participants