?

Success

Related to # 5084
Referenced as Not before: # 4645

User tests: Successful: Unsuccessful:

avatar smanzi
smanzi
11 Nov 2014

This PR fixes errors in JHtmlBootstrap::renderModal() (see: #5084) and extends its functionality

The functionality of the 'backdrop' option is extended to accept the 'static' (string) value, which generates a backdrop that doesn't close the modal when clicked.

New options are introduced:

  • closeButton, boolean, Display modal 'x' close button (default = true)
  • keyboard, boolean, to inhibit (when false) closing the modal with the esc key (default=true)
  • animation, boolean, Slide the modal in from the top of the page (default = true)
  • footer, string, HTML displayed in a proper modal footer (default=none)

Please note that this method can now be used to display a modal containing:

  • an iframe ('url' option)
  • whatever HTML you want ($body parameter)
  • both of them (HTML displayed after the iframe)
avatar smanzi smanzi - open - 11 Nov 2014
avatar jissues-bot jissues-bot - change - 11 Nov 2014
Labels Added: ?
avatar dgt41
dgt41 - comment - 11 Nov 2014

@test pass! Well done Sergio! This is B/C compatible!
Now Joomla complies with all the Bootstrap options
screen shot 2014-11-11 at 7 26 36

avatar smanzi
smanzi - comment - 11 Nov 2014

Test instructions:

Patch whatever JHtmlBoostrap::renderModal() by adding 'backdrop' => false to the $params array.

  • Without this PR the backdrop (grey area around the modal) is still displayed
  • With this PR the backdrop is not displayed

You can also test with 'backdrop' => 'static', which will prevent closing the modal when clicking on the backdrop and the two new parameters 'animation' => false and 'closebtn' => false

avatar smanzi
smanzi - comment - 11 Nov 2014

@dgt41 Thanks! Actually the 'keyboard' => true option still doesn't work... I'm not sure why... :-/

avatar smanzi
smanzi - comment - 11 Nov 2014

@everybody please comment and advice on the fate of the now useless modal() function and if in your opinion I should/can modify the structure of the modal (get rid of the "-container" and move the footer)

For example of "well formed" modals, see: http://getbootstrap.com/2.3.2/javascript.html#modals

avatar dgt41
dgt41 - comment - 11 Nov 2014

@smanzi I say lets stick with the default bootstrap code for backward compatibility issues.But I might be wrong here! If no B/C issues arise from the new mark up I'll say go for it. (maybe in new PR…)

avatar smanzi smanzi - change - 12 Nov 2014
The description was changed
avatar smanzi
smanzi - comment - 12 Nov 2014

I took the bold way and committed more changes, without breaking B/C (hopefully):

  • moved JS to the head
  • fixed esc key not functioning for closing the modal
  • added possibility to add plain HTML markup to the modal-body without using an iframe ("$body" is what "$footer" was before). The url option is... optional!
  • added possibility to add a well formed modal-footer div via the "footer" option
  • the height and width option are checked (if url is specified)
  • deprecated modal() - Please see if done correctly and if we should just delete it instead

Please test again: sorry for the nuisance...

avatar smanzi
smanzi - comment - 12 Nov 2014

Ouch! Travis is picky... complained about comments not starting with uppercase... :-/

avatar smanzi
smanzi - comment - 12 Nov 2014

@dgt41
Good catch: I forgot to declare $script = array(); ... but, puzzled by the fact that the code was working anyway, I investigated a little bit more and found out that in PHP arrays are implicitly created at the first occurrence of a construct of the $var[] = kind. See: http://stackoverflow.com/questions/6853067/are-arrays-implicitly-created-in-php-when-one-of-its-keys-are-assigned-something. To be honest this is something I just discovered and now I'm unsure if I should declare it anyway (for readability) or leave it as it is...

The the lexical scope of the variable $footer (now called $body) is (and was) only local to this function. From a functional point of view nothing is changed: if anybody has used it in the past, the generated markup will not change. I only changed the name to reflect the fact that the associated markup is not (and wasn't) put into a canonical modal-footer but into the modal-body (just after the iframe). As I introduced the possibility to have a canonical modal-footer through the use of the 'footer' option, I thought it would be confusing to have a $footer (which is not) parameter and a 'footer' option, thus I decided to change the name of the former.

avatar smanzi
smanzi - comment - 12 Nov 2014

Forgot to mention this: as far as regards using

$script[] = CODE1
...
$script[] = CODEn
JFactory::getDocument()->addScriptDeclaration(implode("\n", $script));

instead of

JFactory::getDocument()->addScriptDeclaration("
CODE
");

I just copied what is done in other parts of Joomla. I thought it was a kind of house rule, but you are indeed correct. The only plus of this style (I mean the one through the array) is that it is easier to comment out and insert new parts of the script for debugging purpose.
Another plus is that if one would prefer to have a minified script (I personally do...) while maintaining readability in the source, it would be easy to achive that by using:

JFactory::getDocument()->addScriptDeclaration(implode(' ', $script));
avatar smanzi
smanzi - comment - 12 Nov 2014

What I did that potentially breaks B/C (but I don't think it will...) is to eliminate the classless "<div id=\"" . $selector . "-container\">" div that was previously used for targeting in the JS and wasn't adhering to the canonical structure of a bootstrap modal.

If there is a consensus that this is a bad thing, I will reinstate it, but, again, it does break the structure of a canonical bootstrap modal...

Edit: oops! I used wrong markdown... Fixed!

avatar smanzi
smanzi - comment - 12 Nov 2014

I'm re-posting this as edits are not visible in Jissues:

What I did that potentially breaks B/C (but I don't think it will...) is to eliminate the classless "<div id=\"" . $selector . "-container\">" div that was previously used for targeting in the JS and wasn't adhering to the canonical structure of a bootstrap modal.

If there is a consensus that this is a bad thing, I will reinstate it, but, again, it does break the structure of a canonical bootstrap modal...

avatar smanzi smanzi - close - 12 Nov 2014
avatar smanzi smanzi - close - 12 Nov 2014
avatar smanzi smanzi - change - 12 Nov 2014
Status Pending Closed
Closed_Date 0000-00-00 00:00:00 2014-11-12 17:23:32
avatar smanzi
smanzi - comment - 12 Nov 2014

Ooops.. closed by mistake! My bad!

avatar smanzi smanzi - reopen - 12 Nov 2014
avatar smanzi smanzi - reopen - 12 Nov 2014
avatar smanzi smanzi - change - 12 Nov 2014
Status Closed New
avatar vdespa vdespa - change - 13 Nov 2014
Status New Pending
avatar smanzi
smanzi - comment - 13 Nov 2014

I did some more research through the code and what I discovered is:

  • Excluding currently open PRs by @dgt41 this method is called only once, in /libraries/cms/toolbar/button/popup.php
  • In this single call the optional last parameter ($footer, now called $body) is not used
  • also @dgt41 doesn't use the last parameter (actually he does, but always with an empty string...)
  • the id="...-container" div I have eliminated is never used for CSS or script targetting, nowhere
  • the supporting modal() function (now useless) is never called anywhere
  • the possibility that this method is called by 3rd party extensions is extremely thin (to be honest, guys, there are much better modals around...) (OK, this is just an opinion...)

Considering the above I:

  • will kill modal() and let it burn in hell
  • will accept the elimination of the bogus id="...-container" div
  • will eliminate the last parameter and...
  • create a new 'body' option as an addition to the 'url' option to populate the modal-body. If both are specified 'body' will be inserted after the 'url' iframe.
  • create a new 'header' option that will be used to populate the header as an alternative to 'title' and 'closebtn'. If 'header' is specified 'title' and 'closebtn' will have no effect.

Please, do advise me!

I'm bringing this to the attention of: @dgt41 @anibalsanchez (as he is testing related PRs) and... @bakual and @infograf768 (sorry to pull you in, guys...)

avatar anibalsanchez
anibalsanchez - comment - 14 Nov 2014

We have to keep modal() function for BC, and flag it as deprecated (or just call renderModal). It is part of the current JHtmlBootstrap interface.

avatar smanzi
smanzi - comment - 15 Nov 2014

@anibalsanchez Anibal, sorry for the late answer!
Are you sure? modal() was called only by renderModal(), and its function was just to inject some related JS that... didn't function.

I have no problem letting modal() sitting there doing nothing as it did before, but IMHO calling renderModal() from there would be a mistake and it will break B/C...

avatar anibalsanchez
anibalsanchez - comment - 17 Nov 2014

@smanzi Ok, I agree. modal() remains untouched, to avoid any impact on a calling third-party extension, and keeping the current API as deprecated.

avatar smanzi
smanzi - comment - 17 Nov 2014

Perfect! Should I put the deprecate notice for 3.3.7 (as I already did) or 3.4?

avatar smanzi
smanzi - comment - 17 Nov 2014

@anibalsanchez wait a minute... by "untouched" do you mean as it is now (just a return;) or as it was before (broken non functioning JS)?

avatar anibalsanchez
anibalsanchez - comment - 17 Nov 2014

@smanzi , well, both implementations have almost the same effect, and they avoid a fatal error.

return; is clearly a deprecated method, but removes the current behaviour.

Deprecation usually leaves the code untouched until it is removed.

For instance, the "broken non functioning JS" implementation was loading static::framework(), an extension could be inadvertently relaying on this call.

avatar smanzi
smanzi - comment - 17 Nov 2014

OK, I will reinstate the old code, then! (although IMHO the possibility of someone having directly called this from their extension are practically 0, but you're right: better being safe than sorry.)

avatar smanzi
smanzi - comment - 17 Nov 2014

@bakual and/or @infograf768, can you please take a look into this and give a milestone so I can put a correct "deprecated" notice to modal()? Thanks!

avatar dgt41
dgt41 - comment - 22 Nov 2014

@smanzi Am I doing something wrong here? Can’t get the desired output:

The values:

screen shot 2014-11-22 at 11 36 35

And the actual result:

screen shot 2014-11-22 at 11 36 53

avatar smanzi
smanzi - comment - 22 Nov 2014

Dimitris, in your second screenshot I don't recognize my JS at all... Sure you have activated this PR in com_patchtester?

avatar dgt41
dgt41 - comment - 22 Nov 2014

This line is supposed to do the task but it is not working:

$html[] = JHtml::_('bootstrap.renderModal', 'modal-' . $name, $params);

And yes the patch is applied!

avatar smanzi
smanzi - comment - 22 Nov 2014

hmmm... strange... may you copy-paste the HTML generated too?

I've tested it with your PR #4661 where it is called with a different syntax, but as far as I can understand it should be equivalent:

JHtmlBootstrap::renderModal(...)

avatar dgt41
dgt41 - comment - 22 Nov 2014

Can you tested with:

JHtml::_(‘bootstrap.renderModal’, …options);
avatar smanzi
smanzi - comment - 22 Nov 2014

OK, give me about half an hour...

avatar dgt41
dgt41 - comment - 22 Nov 2014

@smanzi Nevermind I just did it with both options in another modal.
'backdrop' => true, false, ‘static’ OK
'animation' => true, false OK
'closebtn' => true, false OK
'keyboard' => true, false OK (on false doesn’t close with esc)

@test success

avatar smanzi
smanzi - comment - 22 Nov 2014

@dgt41 perfect! But... do you still have troubles with that instance or is it OK?

avatar dgt41
dgt41 - comment - 22 Nov 2014

No I tried to use the batch modal but the code is scattered around...

avatar smanzi
smanzi - comment - 22 Nov 2014

OK, let me know if you find any problem. The batch modal sound interesting as it seems to add other handlers on('hide'): this should be useful to test...

avatar dgt41
dgt41 - comment - 22 Nov 2014

One think I will like to see is b/c
Right now I used

                $preview = $baseUrl . '/templates/' . $template . '/template_preview.png';
                $html = '<a href="#' . $template . '-Modal" role="button" class="thumbnail pull-left hasTooltip" data-toggle="modal" title="' .
                    JHtml::tooltipText('COM_TEMPLATES_CLICK_TO_ENLARGE') . '">' . $html . '</a>';
                $html .= JHtml::_('bootstrap.renderModal',
                    $template . '-Modal', array(
                        'url' => $preview,
                        'title' => JText::_('COM_TEMPLATES_BUTTON_PREVIEW'),
                        'height' => '800px',
                        'width' => '800px',
                        'backdrop' => true,
                        'animation' => true,
                        'closebtn' => true,
                        'keyboard' => false
                        )
                    );

But what will happen if i used the following (are all the values where expected? ) :

                $preview = $baseUrl . '/templates/' . $template . '/template_preview.png';
                $html = '<a href="#' . $template . '-Modal" role="button" class="thumbnail pull-left hasTooltip" data-toggle="modal" title="' .
                    JHtml::tooltipText('COM_TEMPLATES_CLICK_TO_ENLARGE') . '">' . $html . '</a>';
                $html .= JHtml::_('bootstrap.renderModal',
                    $template . '-Modal', array(
                        $preview,
                        JText::_('COM_TEMPLATES_BUTTON_PREVIEW'),
                        '800px',
                        '800px',
                        true,
                        true,
                        true,
                        false
                        )
                    );
avatar dgt41
dgt41 - comment - 22 Nov 2014

To make it more clear

     * @param   array   $params    An array of options for the modal.
     *                             Options for the modal can be:
     *                             - title     string   The modal title
     *                             - backdrop  mixed    A boolean select if a modal-backdrop element should be included (default = true)
     *                                                  The string 'static' includes a backdrop which doesn't close the modal on click.
     *                             - keyboard  boolean  Closes the modal when escape key is pressed (default = true)
     *                             - closebtn  boolean  Display modal close button (default = true)
     *                             - animation boolean  Fade in from the top of the page (default = true)
     *                             - footer    string   Optional markup for the modal footer
     *                             - url       string   URL of a resource to be inserted as an <iframe> inside the modal body
     *                             - height    string   height of the <iframe> containing the remote resource
     *                             - width     string   width of the <iframe> containing the remote resource

Did you add new values here?
If yes these have to go last in the array, I think

avatar smanzi
smanzi - comment - 22 Nov 2014

was this working with the previous code? I mean without the keys?

avatar dgt41
dgt41 - comment - 22 Nov 2014

should be, never tried. but if anyone is using it like this this will break their code

avatar anibalsanchez
anibalsanchez - comment - 22 Nov 2014

@test success. Modals work in batch dialog, article versions management and in a third-party extension. Tested in latest staging.

avatar smanzi
smanzi - comment - 22 Nov 2014

Never tried too, but I suspect it will not work...
In any case the API calls for a 'key' => 'value' options array

avatar smanzi
smanzi - comment - 22 Nov 2014

Actually, looking at my (and previous) code it should NOT work: the options array is indexed by its keys, it is NOT positional...

avatar smanzi
smanzi - comment - 22 Nov 2014

Thanks @dgt41 and @anibalsanchez for testing! (don't forget to update the test results in Jssues like I did!!)

avatar anibalsanchez anibalsanchez - test_item - 22 Nov 2014 - Tested successfully
avatar dgt41 dgt41 - test_item - 22 Nov 2014 - Not tested
avatar dgt41 dgt41 - test_item - 22 Nov 2014 - Tested successfully
avatar smanzi
smanzi - comment - 22 Nov 2014

Now that it seems to work and be B/C, let me tell you something: I honestly find the MT modal nicer.
The main difference I see is that the MT is centered and has a prettier close button.
I think I can modify this by:

  • adding a new boolean option 'centered' to have it centered
  • making closebtn a mixed type option with a third value 'icon', to mimick the MT close button

What do you think?

avatar dgt41
dgt41 - comment - 22 Nov 2014

I like the center option :+1: But I will say no to the close button, I like this layout!

avatar smanzi
smanzi - comment - 22 Nov 2014

Good: I'll start working on 'centered'. Not trivial: BS modal is a bitch and a JS will be needed as apparently it can't be done by simple CSS. The JS should also handle the window resize event...

avatar dgt41
dgt41 - comment - 22 Nov 2014

Whatever your plans do them in another PR, this is good to go!

avatar smanzi
smanzi - comment - 22 Nov 2014

@dgt41 yesssssssss: I'll wait for this to be merged! :smile:

avatar brianteeman brianteeman - change - 23 Nov 2014
Rel_Number 5084
Relation Type Related to
avatar brianteeman
brianteeman - comment - 23 Nov 2014

setting rtc

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

avatar brianteeman brianteeman - change - 23 Nov 2014
Status Pending Ready to Commit
avatar smanzi
smanzi - comment - 23 Nov 2014

Great, Brian, thanks!
One point still to be solved is for which rev I should put the deprecation notice for modal(): at this time is set as "3.x", which is not so nice...

avatar phproberto
phproberto - comment - 29 Nov 2014

Please don't merge. I'm preparing a PR against @smanzi's to use layouts to render the modal.

avatar brianteeman brianteeman - change - 29 Nov 2014
Labels Added: ?
avatar phproberto
phproberto - comment - 29 Nov 2014

Please @smanzi check my PR smz#1

That includes the use of layouts to render the modal. People can override the modal or just the parts that they want to override. Available layouts:

  • joomla/modal/main.php: Main modal layout. From there the rest are loaded. Useful if you want to replace all the other layouts with custom layouts for example or to change the main div attributes.
  • joomla/modal/header.php: Modal header
  • joomla/modal/body.php: Modal body
  • joomla/modal/footer.php: Modal footer
  • joomla/modal/iframe.php: Modal iframe. Useful if you want to change the iframe attributes.

So component/template developers can have their custom headers or footers or overriding the bootstrap JHtml it can be used with another frontend framework.

For tests you can use this example code to test the modal:

<a href="#modal-test-modal" role="button" class="btn btn-success" data-toggle="modal">Launch demo modal</a>
<?php
$modalParams = array(
    'title'       => 'Test modal',
    'closeButton' => true,
    'url'         => 'http://joomla.org',
    'height'      => 300,
    'width'       => 600,
    'backdrop'    => true,
    'keyboard'    => true,
    'footer'      => '<div class="alert alert-info">Modal footer</div>'
);
$modalBody = '<div class="alert alert-success">Modal <strong>body!</strong></div>';

echo JHtml::_('bootstrap.renderModal', 'modal-test-modal', $modalParams, $modalBody);
?>
avatar phproberto
phproberto - comment - 29 Nov 2014

BTW I have added the 3.4 as version in the deprecated tag. We can change it if it's not merged then

avatar smanzi
smanzi - comment - 29 Nov 2014

Wow, I received a PR! :smiley:
I added some consideration at smz#1

avatar smanzi
smanzi - comment - 30 Nov 2014

There has been MAJOR mods by @phproberto merged to this...
@brianteeman Please remove RTC: it needs more testing.
@dgt41, @anibalsanchez Please re-test if you can, thanks!

avatar brianteeman brianteeman - change - 30 Nov 2014
Labels Removed: ?
avatar jissues-bot jissues-bot - change - 30 Nov 2014
Labels Added: ?
avatar brianteeman brianteeman - change - 30 Nov 2014
Status Ready to Commit Pending
avatar brianteeman
brianteeman - comment - 30 Nov 2014

Removed RTC

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

avatar smanzi
smanzi - comment - 30 Nov 2014

@brianteeman it seems jissues-bot wants to set this to RTC anyway...

avatar brianteeman brianteeman - change - 30 Nov 2014
Labels Removed: ?
avatar smanzi
smanzi - comment - 30 Nov 2014

@brianteeman seems fixed, thanks!

avatar dgt41
dgt41 - comment - 30 Nov 2014

@smanzi One silly question: with this PR will we be able to render other html data other than iframe?

avatar smanzi
smanzi - comment - 30 Nov 2014

@dgt41 yes! Put your HTML in the last parameter of renderModal() and do not set the 'url' option (I don't like this, but it is for B/C...)

avatar smanzi
smanzi - comment - 30 Nov 2014

@dgt41 Only problem I see when used with plain HTML is that the 'height' and 'width' options are not honored (they are set only against the <iframe>).

@phproberto Can you look at this? My idea is to check into the body layout if either option is set and the 'url' option is not set. In case either add a hardcoded "style =" to the <div class="modal-body"> or manipulate it with JS... Is that OK?

One more issue, IMHO, is that in Isis the padding is set as a percentage (1%): this will be royal pain in the back if and when we decide to implement a "vertical center" option (but on the other hand if we manage it to work it will be an assurance that it will work with any styling...)

avatar dgt41
dgt41 - comment - 30 Nov 2014

@smanzi actually, I think, this is the right thing, modal should follow the dimensions of the html stuff that holds

avatar smanzi
smanzi - comment - 30 Nov 2014

@dgt41 I agree, but somebody could start using 'height' and 'width' with plain HTML and wondering why it doesn't work...

avatar smanzi smanzi - close - 30 Nov 2014
avatar smanzi smanzi - close - 30 Nov 2014
avatar smanzi smanzi - head_ref_deleted - 30 Nov 2014
avatar smanzi smanzi - change - 30 Nov 2014
Status Pending Closed
Closed_Date 2014-11-12 17:23:32 2014-11-30 16:45:14
avatar smanzi
smanzi - comment - 30 Nov 2014

f**k! I made a mistake! hope I can fix...

avatar smanzi smanzi - head_ref_restored - 30 Nov 2014
avatar smanzi smanzi - change - 30 Nov 2014
Status Closed New
avatar smanzi smanzi - reopen - 30 Nov 2014
avatar smanzi smanzi - reopen - 30 Nov 2014
avatar smanzi
smanzi - comment - 30 Nov 2014

pfui.... it seems I managed to fix my mistake (delete wrong branch!!)

avatar phproberto
phproberto - comment - 30 Nov 2014

Maybe I haven't understood it but I don't agree that the modal has to follow the dimensions of its content. Modal should have predefined dimensions set by the framework/tool used to create it.

If you are refering to force the modal size to the received iframe size I think that can make sense. But forcing a modal that won't be responsive sounds bad to me. With current system you can always change the modal size with CSS inside your component applying an style to the modal id. If we apply inline styles it will be hard to override them.

What I'd do maybe is to change the height & width params to something that describes them better like iframeHeight & iframeWidth. Or just add something like iframeAttribs where you can set any attribute you want to pass to the iframe. This for example would allow us to render a google map for example.

I'll play a bit with it tonight :)

avatar smanzi
smanzi - comment - 30 Nov 2014

@phproberto, unhappily the 'height' and 'width' options cannot be renamed without breaking B/C.

I think that when @dgt41 says that "modal should follow the dimensions of the html stuff that holds" he is saying exactly what we all mean, i.e. that there shouldn't be any set dimensions and the modal, by adapting his size to its content, should be responsive (as its content is).

Historically, I think that in the intentions of the original coder this method was meant just for displaying iframes and its last parameter was called "$footer" (not $body). Unlike a "well-mannered" real footer, anyway, its content was not put in its own <div> (like e.g. the "header" does) but just appended to the <iframe> code, inside the .modal-body <div>.

That made me think about re-purposing it to eventually hold some generic HTML (together with or without the <iframe>) and add a properly constructed 'footer' as an option.

This also gives more sense to it having the dignity of a parameter instead of a $params's option.

Please keep in mind that I always had to deal with B/C...

Again thanks for your code which has been a major improvement: this PR should have your name written on it, not mine!

avatar smanzi
smanzi - comment - 30 Nov 2014

... an option could be to have this method to be just an 'iframe specific' proxy (for B/C) to a more generic modal() method. Unhappily the modal() name is already taken for the quite useless broken-js-injecting method...

avatar brianteeman brianteeman - alter_testresult - 30 Nov 2014 - anibalsanchez: Not tested
avatar brianteeman brianteeman - alter_testresult - 30 Nov 2014 - dgt41: Not tested
avatar brianteeman
brianteeman - comment - 30 Nov 2014

reset the test results since there were major changes that need retesting - thanks

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

avatar anibalsanchez
anibalsanchez - comment - 1 Dec 2014

@test success. Modals work in batch dialog, article versions management and in a third-party extension. Tested in J 3.4 Alpha.

avatar anibalsanchez anibalsanchez - test_item - 1 Dec 2014 - Tested successfully
avatar smanzi
smanzi - comment - 1 Dec 2014

Thanks @anibalsanchez !

avatar dgt41
dgt41 - comment - 1 Dec 2014

@test OK

As I was testing this I found a small bug here the last selector should be document.getElementById('batch-tag-id').value=‘’
Do you care to make a PR?

avatar dgt41 dgt41 - test_item - 1 Dec 2014 - Tested successfully
avatar smanzi
smanzi - comment - 1 Dec 2014

@dgt41 thanks for testing!
Will open a PR later tonight for that. Would be "Error in Cancel Button onclick action" be OK as a title?

... and ... careful with those "iQuotes"! :stuck_out_tongue_winking_eye:

avatar smanzi
smanzi - comment - 1 Dec 2014

@brianteeman we have 2 successful tests again here: can you re-set RTC and milestone? tnxs!

avatar smanzi
smanzi - comment - 1 Dec 2014

@dgt41 & @anibalsanchez ehmmm... do you know, guys, that the batch modal is not using this? It seems to me that it is using its own hardcoded modal...

The only reference I find to this method (beside @dgt41 PR) is in: /libraries/cms/toolbar/button/popup.php

avatar dgt41
dgt41 - comment - 1 Dec 2014

Houston we have a problem… Let recheck then

avatar smanzi
smanzi - comment - 1 Dec 2014

@dgt41 your PR (updated) will be greatly appreciated at this point...

but @anibalsanchez has tested this also with a 3rd party extensions...

avatar anibalsanchez
anibalsanchez - comment - 1 Dec 2014

I have also checked other cases: article versions management and in a third-party extension

avatar dgt41
dgt41 - comment - 1 Dec 2014

#4575 is still good

avatar smanzi
smanzi - comment - 1 Dec 2014

I'm afraid article management too is doing its things the hard way. Same in com_finder indexer, I guess.
the file I cited is the only one that does bother using the official modal...

avatar smanzi
smanzi - comment - 1 Dec 2014

may I ask you, then, to test this with #4575, just to be sure that it gets through?

avatar smanzi
smanzi - comment - 1 Dec 2014

when this thing will be merged I think an extensive code review of all used modals should follow...

avatar dgt41
dgt41 - comment - 1 Dec 2014

I had to apply #5259 with #4575. So:
'backdrop' => true, false OK (static don’t work here, in the previous iteration was working)
'animation' => true, false OK
'closebtn' => true OK (false doesn’t hide the x, but x doesn’t close the modal. Is this the way it should be?)
'keyboard' => true, false OK (on false doesn’t close with esc)

avatar dgt41
dgt41 - comment - 1 Dec 2014

screen shot 2014-12-01 at 9 05 33

avatar smanzi
smanzi - comment - 1 Dec 2014

@dgt41 No: we have problems. Will review... damn!

avatar dgt41
dgt41 - comment - 1 Dec 2014

Sorry the close button false doesnt work (x still there and still functional)

avatar smanzi
smanzi - comment - 1 Dec 2014

@dgt41 ah, wait, I probably know why 'closebtn' is not working: @phproberto changed that option name to 'closeButton' :smile:

Will review 'backdrop' => 'static'...

avatar smanzi
smanzi - comment - 1 Dec 2014

@dgt41, @anibalsanchez
Tested with #5259 + #4575 and everything seems to work (I modified #4575 to test the various options).

Remember:

  • new option for Close Button is 'closeButton'
  • for static backdrop the option must be 'backdrop' => 'static' (mind the quotes on 'static': it is a string...)
  • but: 'backdrop' => true or 'backdrop' => false (boolean)
avatar smanzi
smanzi - comment - 1 Dec 2014

@phproberto the CSS for .modal-footer in Isis is quite objectionable... I don't know what to do with that...

avatar smanzi
smanzi - comment - 7 Dec 2014

@phproberto Roberto, I have the feeling this PR will not be merged until the whole JHtmlBootstrap class will be "layout-enabled".

As my initial version, albeit a lot less elegant, did anyway solved a bug in the modal (and was already tested by two testers), I'm wondering if it will not preferable to merge that version first, and at second time substitute it with your layout-enabled version...

Of course I'll do the required changes to make it 100% compatible with your code (if I'm not mistaken this means just changing the name of my original 'closebtn' option to your 'closeButton' wording...)

Thanks!

avatar dgt41
dgt41 - comment - 7 Dec 2014

@smanzi Please don’t do that, splitting it to two parts is a lot more work for all of us (coding testing)!
I will retest it again.
Also #4575 is also updated now so if you copy paste here the code and some instructions for the tests this will be easy to gets some tests…

avatar dgt41
dgt41 - comment - 7 Dec 2014

@test success
I used patch tester with 4575 and then used this code to test all switches:

<?php
//echo JHtmlBootstrap::renderModal('multiLangModal', array( 'url' => $link, 'title' => JText::_('MOD_MULTILANGSTATUS'), 'height' => '300px', 'width' => '500px' ));

echo JHtml::_('bootstrap.renderModal',
        'multiLangModal', array(
        'url' => $link,
        'title' => JText::_('MOD_MULTILANGSTATUS'),
        'height' => '300px',
        'width' => '400px',
        'backdrop' => true,
        'animation' => true,
        'closeButton' => false,
        'keyboard' => true
    )
);

Possible values to test:
'backdrop' => true, false, ’static'
'animation' => true, false
'closeButton' => true, false
'keyboard' => true, false

avatar smanzi
smanzi - comment - 7 Dec 2014

@dgt41 Dimitris, as far as regards API and generated HTML my old code and the new code by @phproberto are 100% compatible (once changed that option name), so I don't think it will be a drama if mine would be delivered as a first step (testing is quite an easy thing...)

The rationale for me asking if it would be better (not proposing) to make this PR a two-steps-thing is that at the current stage renderModal() will be the only method in JHtmlBoostrap leveraging the layouts architecture while it would be probably better that also all other methods will use the same architecture. My guts are saying that this will mean this PR not being merged at 3.4 and delayed at 3.5. I think this will be a pity because we will leave a bug in Joomla for which we have a solution ready (actually we have two solutions!)

Just to be clear: I have nothing against @phproberto implementation: as I said and truly believe it is a better solution than mine, but I would like to see a solution (Roberto's one, mine, or whomever's else) delivered with Joomla 3.4. That's it! :smile:

avatar smanzi
smanzi - comment - 7 Dec 2014

@dgt41 thanks for testing!

avatar dgt41
dgt41 - comment - 7 Dec 2014

@smanzi 3.5 will be a better fit, since all the layouts and modal will change then. Also right now in the backend BS modals are kinda hardcoded and don’t follow these changes, and in front end nobody (ok exaggerating here) is using BS 2.3.2. Although will be nice to have it, since it’s ready, the option to merge this right after 3.4 won’t change something. My point: this is OK don’t try to rush it by changing it, just to have it in 3.4, if won’t make it in 3.4 will make it in 3.4.1… Also even if you change it to a simpler implementation I don’t think that this will guaranty the inclusion in 3.4.
My 2c.
PS Invite someone to test it so this will become rtc and get chances for 3.4...

avatar smanzi
smanzi - comment - 7 Dec 2014

OK, Dimitris: IMHO it is always bad to leave bugs behind, but I will not insist further.
As far as regards finding another tester, @anibalsanchez has already given a go to this (in the current implementation), so I think we are all-set.

avatar anibalsanchez
anibalsanchez - comment - 8 Dec 2014

@test success! Tested with a JToolBarHelper::preview in a toolbar.

avatar Hackwar
Hackwar - comment - 8 Dec 2014

@smanzi If you want to deprecate something, add the version number when it will be removed from Joomla to the deprecated tag. So deprecating for example modal() would mean @deprecated 4.0. That is the earliest that we can remove it.

avatar smanzi smanzi - change - 8 Dec 2014
The description was changed
avatar smanzi
smanzi - comment - 8 Dec 2014

@Hackwar Fixed, thanks! Yes, you're right: for no apparent reason, but it will stay there until 4.0

avatar smanzi
smanzi - comment - 8 Dec 2014

@Hackwar Anyway, my personal opinion is that we should use the @deprecated tag to identify the version since when we are advising against the use of something and not when we forecast to remove it.

After all (see http://manual.phpdoc.org/HTMLSmartyConverter/HandS/phpDocumentor/tutorial_tags.deprecated.pkg.html) @deprecated is normally associated to the adverb 'since' and 'to deprecate' != 'to remove'

avatar Hackwar
Hackwar - comment - 8 Dec 2014

That might be, but from history we know that we tend to then not find all deprecated areas correctly when we mark them as "deprecated since". That is why Joomla adopted to mark it as "This is deprecated and will be removed in that version"

avatar smanzi
smanzi - comment - 8 Dec 2014

Makes sense, but I propose to review our policy and... deprecate our use of @deprecated 'since' version 4.0! :smile:
I think we should use something like that:

@deprecated since 3.4 - Will be removed in 4.0
avatar dgt41
dgt41 - comment - 8 Dec 2014

@deprecated 3.4->4.0less code ????.
Anyways this might be irrelevant by 4.0 as the target is to get BS 3 before version 4, so all this will be history way before...

avatar mbabker
mbabker - comment - 8 Dec 2014

Actually, the syntax of @deprecated 4.0 is a pretty common PHP standard (soon to be more formally adopted in the form of PSR-5, php-fig/fig-standards#169) where the tag lists the first version in which the deprecated code may be removed. Adopting something like you've suggested, although it would read many times better, actually would be quite tricky when working with third party tools; at least for the deprecated tag, we would have to override the tag parsers in phpDocumentor (what we're using to generate api.joomla.org) to deal with our "unique" standards.

avatar smanzi
smanzi - comment - 8 Dec 2014

I guess you are referring to this:

+ @deprecated [<"Semantic Version">] [<description>]
+
+#### Description
+
+The @deprecated tag declares that the associated 'Structural elements' will
+be removed in a future version as it has become obsolete or its usage is
+otherwise not recommended.
+
+This tag MAY also contain a version number up till which it is guaranteed to be
+included in the software. Starting with the given version will the function be
+removed or may be removed without further notice.

then I agree, but let's drop the word 'since': I'm not of English mother tongue but my strong feeling is that since can't refer to something in the future...

... and also 'until' is ambiguous: is it inclusive or exclusive? Look here: http://www.quickanddirtytips.com/education/grammar/does-until-include-the-date

avatar zero-24 zero-24 - change - 15 Dec 2014
Status New Pending
avatar dgt41
dgt41 - comment - 17 Jan 2015

@roland-d can you merge this one?

avatar roland-d roland-d - change - 19 Jan 2015
Status Pending Ready to Commit
avatar roland-d
roland-d - comment - 19 Jan 2015

Moving to RTC as we have 2 successful tests.


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

avatar roland-d roland-d - close - 19 Jan 2015
avatar roland-d roland-d - close - 19 Jan 2015
avatar roland-d roland-d - change - 19 Jan 2015
Status Ready to Commit Closed
Closed_Date 2014-11-30 16:45:14 2015-01-19 09:25:28
avatar roland-d roland-d - reference | a721653 - 19 Jan 15
avatar roland-d roland-d - change - 19 Jan 2015
Milestone Added:
avatar smz smz - head_ref_deleted - 29 May 2015

Add a Comment

Login with GitHub to post a comment