Closed nykopol closed 9 years ago
@Ingannatore can you review this? It looks fine to me.
Sorry for the long delay; I'm checking your pull request right now.
If I disable assets (include_assets: false
), the bundle won't even include the mandatory "ready" and "init" js files; is this the correct behaviour of your pull request, @nykopol?
Sorry but your changes break the system: if I manually disable the assets inclusion by setting the include_assets
option to false
, the bundle won't even include the mandatory "ready" and "init" js files.
maybe my intention was not clearly explained. What I wanted to allow was :
tinymce_assets()
tinymce_init()
But there is maybe a better way to do that ( #14 ?)
To test your pull request I set the include_assets
option to false
; as per documentation, the bundle should not include jQuery nor TinyMCE but it should still include the bundle's mandatory JavaScript files: ready.min.js
and init.standard.js
(if working without jQuery) or init.jquery.js
(if working with jQuery).
I confirm that your pull request limit the assets inclusion to one copy, but it works only if include_assets
is set to true
. If include_assets
is set to false
, your code (tinymceAssets
method inside src/Twig/Extension/StfalconTinymceExtension.php
) does the following:
if(!$this->include_assets) {
return "";
}
Thus bypassing the assets inclusion; check the Resources/views/Script/assets.html.twig
file and you'll see that it contains also the above mandatory files.
IMO, if you set include_assets
to false
, you know that assets will not be loaded so you have to handle that by yourself.
For me, if we want to clarify the situation, we should maybe throw a LogicException rather than return an empty string when the function tinymce_assets()
is called with the option include_assets
to false
.
I though that for "assets" you meant the externals JS dependencies, as per your changes to the how-to guide:
The option
include_assets
allows you to controle if assets (jQuery and TinyMCE) must be loaded. Set it tofalse
if you include theme already by yourself.
Back to your last comment, I'm unable to find any usefulness in your method: if, by setting include_assets
to false
, I have to manually add and initialize everything, what is the point of even using this bundle? @zanardigit: can I have your opinion on the matter?
@nykopol I understand your need but there are a few thing to fix in the PR, specifically about reusing the configuration object and how exactly to deal with the "false" value. I'll have a look at this as soon as possible.
This PR allow to load separately assets and TinyMCE instantiation. This allow for example the possibility to load assets only once if we have 2 editors on the same page.