? Success

User tests: Successful: Unsuccessful:

avatar C-Lodder
C-Lodder
26 Mar 2015

What this PR does

It simply removes the unnecessary banner.js that was being imported and instead, uses Joomla's addScriptDeclaration which was already being used.

How to test

  1. In the Joomla backend, open an existing or create a new banner.
  2. Switch between Custom and Image for the parameter labelled Type

Expected result

When the Type is set to Custom, the following parameter will appear:

  • Custom Code

When the Type is set to Image, the following parameters will appear:

  • Image
  • Width
  • Height
  • Alternative text

Image preview

banners

avatar C-Lodder C-Lodder - open - 26 Mar 2015
avatar joomla-cms-bot joomla-cms-bot - change - 26 Mar 2015
Labels Added: ?
avatar joomla-cms-bot joomla-cms-bot - change - 26 Mar 2015
Labels Added: ?
avatar n9iels
n9iels - comment - 26 Mar 2015

@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?..)

avatar C-Lodder
C-Lodder - comment - 26 Mar 2015

@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.

avatar n9iels
n9iels - comment - 26 Mar 2015

Thanks for the explanation :smiley: , in that case:
@test Works like expected :+1:


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/6588.
avatar n9iels n9iels - test_item - 26 Mar 2015 - Tested successfully
avatar wilsonge
wilsonge - comment - 27 Mar 2015

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?

avatar dgt41
dgt41 - comment - 27 Mar 2015

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

avatar C-Lodder
C-Lodder - comment - 27 Mar 2015

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.

avatar dgt41
dgt41 - comment - 27 Mar 2015

@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 :+1:

avatar zero-24 zero-24 - change - 27 Mar 2015
Easy No Yes
avatar zero-24 zero-24 - change - 27 Mar 2015
Category JavaScript
avatar Dylis
Dylis - comment - 9 May 2015

@test - Success. Component banner still working properly after applying patch, both Image and Custom options displaying correctly.


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/6588.

avatar Dylis Dylis - test_item - 9 May 2015 - Tested successfully
avatar zero-24 zero-24 - change - 17 Jun 2015
Status Pending Ready to Commit
avatar zero-24 zero-24 - change - 17 Jun 2015
Labels Added: ?
avatar wilsonge wilsonge - change - 4 Jul 2015
Status Ready to Commit Closed
Closed_Date 0000-00-00 00:00:00 2015-07-04 01:57:48
Closed_By wilsonge
avatar wilsonge wilsonge - close - 4 Jul 2015
avatar zero-24 zero-24 - close - 4 Jul 2015
avatar wilsonge
wilsonge - comment - 4 Jul 2015

Merged - thanks Charlie

avatar zero-24 zero-24 - change - 14 Oct 2015
Labels Removed: ?

Add a Comment

Login with GitHub to post a comment