Update Assets.php #1687
Update Assets.php #1687
Conversation
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>
```
system/src/Grav/Common/Assets.php
Outdated
| @@ -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'] == '')) { | |||
There was a problem hiding this comment.
Should be rewritten as:
if ($group && $inline['group'] === $group && $inline['type'] === '') {as per recommendation
|
Requested changes applied |
|
What about the ordering? I'm a bit uneasy as the assets are not added in the order they were added by the code. |
|
Indeed. The cost is that if several inlined js is added, it will output multiple script tags but it is valid. What do you think? |
|
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. |
|
@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. |
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
It will be rendered like this:
With this fix, it should display