Skip to content

Move editor plugin buttons modals to Bootstrap#5652

Closed
dgrammatiko wants to merge 6 commits intojoomla:stagingfrom
dgrammatiko:_plugins_buttons_bs_modal
Closed

Move editor plugin buttons modals to Bootstrap#5652
dgrammatiko wants to merge 6 commits intojoomla:stagingfrom
dgrammatiko:_plugins_buttons_bs_modal

Conversation

@dgrammatiko
Copy link
Copy Markdown
Contributor

@dgrammatiko dgrammatiko commented Jan 9, 2015

This is a redone for #4664 that uses the jModalClose function introduced #3918 from @okonomiyaki3000

What is changed?

Well this PR leaves the current layouts of the editor plugin button untouched in the root folder of Joomla (this is done for B/C) but introduces the usage of bootstrap for Isis template.
The actual rendering of the buttons:

screen shot 2015-01-09 at 4 35 03
screen shot 2015-01-09 at 4 35 14
screen shot 2015-01-09 at 4 35 26

There are two questions here:

@wilsonge Shall I copy the button layout into protostar as well?

@okonomiyaki3000 I made some changes in behavior.php regarding jModalClose (due to the way bootstrap script is working):

        // Attach modal behavior to document
        $document
            ->addScriptDeclaration('
        jQuery(function($) {
            SqueezeBox.initialize(' . $options . ');
            SqueezeBox.assign($("' . $selector . '").get(), {
                parse: "rel"
            });
        });
        if(typeof jModalClose == "function"){
            var fnCode = jModalClose.toString() ;
            fnCode = fnCode.replace(/\}$/, "SqueezeBox.close();\n}");
            window.eval(fnCode);
        }
        else {
            function jModalClose() {
                SqueezeBox.close();
            }
        }'
        );

Is that ok? Especially this part: window.eval(fnCode);

@okonomiyaki3000
Copy link
Copy Markdown
Contributor

okonomiyaki3000 commented Jan 10, 2015

I had in mind a different way to override the default modal with a different type. The thing is, JHtmlBehavior::modal, such as it is, is all about using SqueezeBox. If you don't want to use SqueezeBox, it's best to completely override that function with one of your own by using JHtml::register. In other words, you don't need to touch behavior.php.

I guess you're trying to have it such that both kinds of modals are available. I suppose I don't really agree with that idea. If you want to use bootstrap modals (good!) then why not replace SqeezeBox in all cases?

@dgrammatiko dgrammatiko mentioned this pull request Jan 10, 2015
Merged
@dgrammatiko
Copy link
Copy Markdown
Contributor Author

dgrammatiko commented Jan 12, 2015

@okonomiyaki3000 I can understand the way you meant this code to be used, totally replace the modal, and of course is a good approach. The point in this particular case, and I guess to some other places in core, is that you might end up having the core with all modals using bootstrap but some old 3pd component will use the mootools modal.
I think this is all about B/C and how we can make different frameworks work together (especially with the upcoming layouts)
I am afraid we end up with two solutions here:

  1. Solve it in client side with some code like this one I used here
  2. Solve it in server side:
    Introduce a function addScriptModalClose (which creates an array similar to _scripts)
    Call it and set the code for each modal
    On head.php implode the array and render the proxy function

Of course I might be very wrong with my assumptions here and there might be a way easier solution. But then again thats the reason why I raise this concern...

@dgrammatiko dgrammatiko mentioned this pull request Jan 13, 2015
Closed
@dgrammatiko dgrammatiko mentioned this pull request Jan 22, 2015
Closed
@dgrammatiko
Copy link
Copy Markdown
Contributor Author

dgrammatiko commented Feb 24, 2015

This is for 3.5!!!
In order to successfully test this one you need to apply first #5652.
@phproberto Can you take a look at this one?
The problem here is the way we handle the scripts and the closing function of bootstrap modal.
This might be a slight B/C issue for people already using layouts of those buttons, but I cannot figure out a totally compatible solution. Maybe you can think of something that will do the trick ♻︎

dgrammatiko
dgrammatiko reviewed Feb 27, 2015
View reviewed changes
layouts/joomla/editors/buttons.php Outdated
Copy link
Copy Markdown
Contributor Author

@dgrammatiko dgrammatiko Feb 27, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@wilsonge Will the new layouts renderer have some problems with this code above?

Copy link
Copy Markdown
Contributor

@wilsonge wilsonge Feb 27, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Umm what are you trying to do? If it's create template override paths for the JLayouts then we'll be doing that in JPlugin (unsure as exact paths but using the add include path(s) https://github.com/joomla/joomla-cms/blob/staging/libraries/cms/layout/file.php#L159-L195 functions)

Copy link
Copy Markdown
Contributor Author

@dgrammatiko dgrammatiko Feb 27, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem is that these buttons have some js and some code for closing the modals. Right now only few params are passed to layout and then the button gets rendered. If we are about to eliminate mootools we need to set the correct javascript (modal close is the problem) in the layout. This code is a simple fall back for 3rd party plugins to keep working

@dgrammatiko
Copy link
Copy Markdown
Contributor Author

dgrammatiko commented Feb 27, 2015

Needs also #5871

@ghost
Copy link
Copy Markdown

ghost commented Mar 14, 2015

Patch breaks ability to insert image.

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

@dgrammatiko
Copy link
Copy Markdown
Contributor Author

dgrammatiko commented Mar 14, 2015

@Nueckman try to apply also #5871

@dgrammatiko
Copy link
Copy Markdown
Contributor Author

dgrammatiko commented Mar 14, 2015

@Nueckman works here, see the video

@dgrammatiko dgrammatiko mentioned this pull request Mar 27, 2015
Closed
@joomla-cms-bot joomla-cms-bot added the Language Change This is for Translators label Jun 3, 2015
@dgrammatiko
Copy link
Copy Markdown
Contributor Author

dgrammatiko commented Jun 9, 2015

@phproberto if you have some spare time can you review this one? Reminder it needs #5871 as well (and if you also apply #5654 you 'll drop mootools completely in front end)

@dgrammatiko dgrammatiko mentioned this pull request Jun 12, 2015
Closed
@dgrammatiko dgrammatiko deleted the _plugins_buttons_bs_modal branch August 14, 2015 16:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

Language Change This is for Translators

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@dgrammatiko @okonomiyaki3000 @wilsonge @jissues-bot @joomla-cms-bot