Skip to content

Conversation

@phproberto
Copy link
Contributor

This solves #5254

Issue description

I was checking core.js and noticed that we have added there some jQuery code in functions:

Joomla.renderMessages
and
Joomla.removeMessages

They are mainly used on install and overrider but also third part developers can be using them.

We officially support jQuery & Mootools even being migrating to jQuery. Adding that code causes that developers that want to use only Mootools are forced to load jQuery.

So we have to convert them to plain JS.

How to test

As it's hard to test message rendering in installation & language overrides here is some code that you can use. You have to add this code before applying the patch and test it before and after the patch to be sure that it's working the same.

I will explain how to put testing code in the articles view ( administrator/components/com_content/views/articles/tmpl/default.php). Around line 54 add:

JText::script('ERROR');
JText::script('MESSAGE');
JFactory::getDocument()->addScriptDeclaration('
    jQuery(document).ready(function() {
        jQuery(".js-test").click(function(){
            var messages = {
                "message": ["Message one", "Message two"],
                "error": ["Error one", "Error two"]
            };
            Joomla.renderMessages(messages);
        });
        jQuery(".js-test-clear").click(function(){
            Joomla.removeMessages();
        });


    });
');

Then before line 66 (after the main container tag) add the buttons that will trigger the actions:

<a class="btn btn-success js-test" href="#">Render messages</a>
<a class="btn btn-danger js-test-clear" href="#">Clear messages</a>

You will see something like:
buttons

  • Clicking on the Render messages button should display the messages.
  • Clicking on the Clear messages button should remove the messages.

View with the messages loaded:

messages

@dgrammatiko
Copy link
Contributor

@test success
screen shot 2014-11-30 at 4 33 29
screen shot 2014-11-30 at 4 33 38

When deleting the msgs we end up with a lot of white space, I guess this happens only in the test env?

@phproberto
Copy link
Contributor Author

In fact it was a chrome bug :(

Thanks for testing it should work now.

@dgrammatiko
Copy link
Contributor

will retest in the morning

@Fedik
Copy link
Member

Fedik commented Nov 30, 2014

test
works good 👍

@dgrammatiko
Copy link
Contributor

@test success

screen shot 2014-11-30 at 3 33 40
screen shot 2014-11-30 at 3 33 25

@wilsonge
Copy link
Contributor

Can we remove the call to jQuery in JHtmlBehaviour::core(); as part of this then please?

@dgrammatiko
Copy link
Contributor

@wilsonge But JHtmlBehaviour::core(); is out in the wild and people might use it to load both core+jquery. We can introduce JHtmlBehaviour::joomla(); or something else for clean core.
My 2c on this

@Bakual
Copy link
Contributor

Bakual commented Nov 30, 2014

Can we remove the call to jQuery in JHtmlBehaviour::core(); as part of this then please?

If core.js doesn't need jQuery anymore, then remove the call to jQuery.

But JHtmlBehaviour::core(); is out in the wild and people might use it to load both core+jquery. We can introduce JHtmlBehaviour::joomla(); or something else for clean core.

Nah. JHtmlBehaviour::core(); is supposed to load core.js. It happens to depend on jQuery for now, but if developer depend on it loading jQuery, then they are wrongly doing that.
It's the same thing we have with MooTools. Developers should always load their dependencies and not trust that other code does that for you.

@dgrammatiko
Copy link
Contributor

@wilsonge @Bakual Removing https://github.com/joomla/joomla-cms/blob/staging/libraries/cms/html/behavior.php#L88 didn’t broke something, so I guess you are both right here!

@phproberto
Copy link
Contributor Author

Good catch @wilsonge :)

I think this is perfect now.

@dgrammatiko
Copy link
Contributor

@phproberto don’t commit this one yet… I am sending you a PR...

@dgrammatiko
Copy link
Contributor

@phproberto
Copy link
Contributor Author

I merged @DGT41's PR 👍

@Bakual
Copy link
Contributor

Bakual commented Dec 4, 2014

Merged, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants