? Success

User tests: Successful: Unsuccessful:

avatar dgt41
dgt41
15 Nov 2014

Executive summary

This PR converts the form validation on com_wrapper to use plain jquery (no mootools call on every form).
Also NO MORE INLINE SCRIPTS!

Testing

  1. Apply first PR #4888 (!important)
  2. Apply this PR
  3. In the admin area create new menu items for wrapper
  4. go to the new menu and test that everything is still OK
  5. In Admin, Create a new module wrapper and publish it
  6. Check in front end that the module displays correctly

If no javascript errors are logged in your browser’s console and the functionality remains the same, your test is passing in any other case please report the errors here

Please also check these:
administrator/index.php?option=com_checkin should demonstrate multiselect without mt
administrator/index.php?option=com_users&view=mail should demonstrate form sent and validate without mt
administrator/index.php?option=com_modules should demonstrate multiselect and combobox without mt
Logout and log in to demonstrate the use of noframes without mt.

I think I’m done here Joomla is now a lot less dependent on mootools
~there are still some mootools stuff in the core, I guess we will tap them later on~

c64dcb2 15 Nov 2014 avatar dgt41 init
avatar dgt41 dgt41 - open - 15 Nov 2014
avatar jissues-bot jissues-bot - change - 15 Nov 2014
Labels Added: ?
avatar zero-24 zero-24 - change - 16 Nov 2014
Category Front End JavaScript
avatar anibalsanchez
anibalsanchez - comment - 17 Nov 2014

@test success, in Firefox and Chrome, loading a wrapped Url

avatar roland-d roland-d - alter_testresult - 19 Nov 2014 - anibalsanchez: Tested successfully
avatar roland-d
roland-d - comment - 19 Nov 2014

@smanzi Would you mind testing this one also? Thank you.

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

avatar smanzi
smanzi - comment - 24 Nov 2014

@test fail!

in the Wrapper module the Height is botched: it adds "60" (literally, not numerically) to the indicated height, so 200 becomes 20060px, 130 becomes 13060px, etc...

I guess the intent was to add 60px (why???)

There are other calculations that probably have the same bug.

avatar smanzi
smanzi - comment - 24 Nov 2014

can confirm that without this PR the issue is NOT present... (strange: looking at the diff I'd bet it was already there....)

avatar smanzi
smanzi - comment - 24 Nov 2014

I think the reason is simply that previously the botched (and apparently useless) code wasn't run at all!
Now, with your correction to the script declaration the code is executed and does its harm...

avatar dgt41
dgt41 - comment - 24 Nov 2014

Nice :+1: That was my mistake, adding string with number

avatar smanzi
smanzi - comment - 24 Nov 2014

it was already there... but not executed, I guess!

avatar smanzi
smanzi - comment - 24 Nov 2014

there are others like that...

avatar dgt41
dgt41 - comment - 24 Nov 2014

Should be fine now...

avatar smanzi
smanzi - comment - 24 Nov 2014

Now it works, but... I declare I want an height of 200 and I get 260. Why???? I think this was to support some old template. Then it got forgotten and wasn't working anyway (so I did get a 200px height): now I get 260!!

avatar smanzi
smanzi - comment - 24 Nov 2014

... and in a way it breaks B/C...

avatar smanzi
smanzi - comment - 24 Nov 2014

I'll just get rid of that silly JS tampering with the height...

avatar dgt41
dgt41 - comment - 24 Nov 2014

The height calculations here are for the title

avatar dgt41
dgt41 - comment - 24 Nov 2014

And honestly I don’t see the use

avatar smanzi
smanzi - comment - 24 Nov 2014

... and the module creates an iframe with a fixed ID 'blockrandom': what if I put two or more wrapper modules in a page? Broken HTML!

avatar smanzi
smanzi - comment - 24 Nov 2014

fuck the title! the title can have any height, depending on how I style it with my CSS...

avatar smanzi
smanzi - comment - 24 Nov 2014

The fixed ID is a royal bug. Apart the silly name (blockrandom? where this was taken from??), you can't have two elements with the same ID on a page, but I can have two Wrapper Modules on a page, so if we need an ID (and I would strongly vote NO for it), at least add the moduleId to it to make it unique... (I'm not blaming you!!)

avatar dgt41
dgt41 - comment - 24 Nov 2014

I guess this is more meaningful: id="<?php echo $target; ?>”`

avatar smanzi
smanzi - comment - 24 Nov 2014

could it be that this comes directly from Mambo and they had a Mambot changing 'blockrandom' to something really random?

avatar smanzi
smanzi - comment - 24 Nov 2014

IMHO ID is designer territory...

avatar dgt41
dgt41 - comment - 24 Nov 2014

Maybe a good idea is to ditch the javascript altogether here. We just need to set some iframe properties and thats all!

avatar smanzi
smanzi - comment - 24 Nov 2014

:+1:

What's in $target?

avatar dgt41
dgt41 - comment - 24 Nov 2014

The tooltip says Name of the iFrame when used as Target. There is an field you can put a name in the admin area /module /wrap

avatar smanzi
smanzi - comment - 24 Nov 2014

ah yes, seen... and it is blank by default and we normally add name="" this too should be fixed: do not add if empty.

I'll ditch the ID too and call it a day! or, yes, if you wish, set it to $target if $target is set...

avatar dgt41
dgt41 - comment - 24 Nov 2014

Lets see what the masters say @infograf768 @Bakual Do we really need javascript for the wrapper?

avatar smanzi
smanzi - comment - 24 Nov 2014

:stuck_out_tongue_winking_eye: :+1:

avatar smanzi
smanzi - comment - 24 Nov 2014

'good night!

avatar dgt41
dgt41 - comment - 24 Nov 2014

????

avatar Hackwar
Hackwar - comment - 8 Dec 2014

First of all: When is iFrameHeight() even executed? Otherwise, I personally would drop the JS, but there is a large part of the community, that wants iframes to dynamically adapt themselfs to its content. So if you want to do this right, attach an event that changes this continously. I would use jQuery for that...

avatar Hackwar
Hackwar - comment - 8 Dec 2014

Oh, and make it optional and drop that +60 or +20 height.

avatar dgt41
dgt41 - comment - 6 Jan 2015

I would redo both component and module the following days, thus I am closing this one

avatar dgt41 dgt41 - close - 6 Jan 2015
avatar dgt41 dgt41 - close - 6 Jan 2015
avatar dgt41 dgt41 - change - 6 Jan 2015
Status Pending Closed
Closed_Date 0000-00-00 00:00:00 2015-01-06 10:52:46
avatar dgt41 dgt41 - head_ref_deleted - 3 Mar 2015

Add a Comment

Login with GitHub to post a comment