User tests: Successful: Unsuccessful:
It simply removes the unnecessary banner.js that was being imported and instead, uses Joomla's addScriptDeclaration
which was already being used.
When the Type is set to Custom, the following parameter will appear:
When the Type is set to Image, the following parameters will appear:
Labels |
Added:
?
|
Labels |
Added:
?
|
@n9iels - I was going to use Joomla's showon="foo:1" but because the fields belong to different fieldset, this didn't seem to be working as expected. Regarding the actual patch, there isn't supposed to be any difference. I've just removed the unnecessary JS file that was being included and merged that code with the injected JS that was in the view file.
Thanks for the explanation , in that case:
@test Works like expected
I believe that @dgt41 has been looking to move scripts into JS files rather than as inline scripts as per the comments at #5839 (comment)
Would it be possible to move all of the javascript into the external file rather than having it inline?
A good measure for stand alone scripts can be the existence of variables passed from php. If no variables are passed then the code can go to a .js file in /media/com_what_ever_the_name_of_the_component/js/some_name.js
There are pros and cons here
PROS
IDE friendly files, better formatting
files are cached
files can be overridden in template if JHTML is used for loading
CONS
More http requests
If the file is changed and loaded without addScripVersion(), then the user needs to clean the browser cache (By the way we need to get this versioning option to JHTML: sounds like an upcoming PR)
Final thought: Please always use the API for scripts, inline or not! This ensures that your code will never break someone else’s code. This is what all those PRs of mine are all about. Hopefully now all these make some sense
By API, I assume you're referring to addScriptDeclaration
, in which case this already uses it. If not then could you let me know what you're referring to.
I'm happy putting all of this in the view or in the separate JS file.
@C-Lodder Yes API is used here since 3.4.0 C-Lodder@dc30c2a
You could alternatively put these javascript lines in a js file (also a.min.js will be required) in /media/com_banners/js/edit.js (You can come up with a better naming…)
But even as it is is right now, is good enough for me. Thanks
Easy | No | ⇒ | Yes |
Category | ⇒ | JavaScript |
@test - Success. Component banner still working properly after applying patch, both Image and Custom options displaying correctly.
Status | Pending | ⇒ | Ready to Commit |
Labels |
Added:
?
|
Status | Ready to Commit | ⇒ | Closed |
Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2015-07-04 01:57:48 |
Closed_By | ⇒ | wilsonge |
Merged - thanks Charlie
Labels |
Removed:
?
|
@test the hide/show is already build in in Joomla! 3.4.1 master. I see no difference when applying this patch. (Not able to test this for 3.4.1 staging, but I think in staging this is also build in?..)