Skip to content

Comments

[4.0] Fix the no jQuery mode#32240

Merged
wilsonge merged 2 commits intojoomla:4.0-devfrom
dgrammatiko:4.0-dev___vanilla-mode
Feb 2, 2021
Merged

[4.0] Fix the no jQuery mode#32240
wilsonge merged 2 commits intojoomla:4.0-devfrom
dgrammatiko:4.0-dev___vanilla-mode

Conversation

@dgrammatiko
Copy link
Contributor

Pull Request for Issue # .

Summary of Changes

  • DRY code, reduced some repeated code for the jQuery mode
  • Ensure that the vanilla mode for the events is called before instantiating any component

Testing Instructions

  • Apply the PR
  • Edit administrator/templates/atum/joomla.asset.json and add "jquery" after line 48
  • Goto administrator dashboard open the browser's console in the and paste:
jQuery
x = document.querySelector('.dropdown-toggle')
jQuery(x).dropdown()

You should see an error: Uncaught TypeError: jQuery(...).dropdown is not a function
The test is successful

Actual result BEFORE applying this Pull Request

The first component could be instantiated using the jQuery methods

Expected result AFTER applying this Pull Request

No component could be instantiated using the jQuery methods

Documentation Changes Required

There is documentation here: #32239

@joomla-cms-bot joomla-cms-bot added NPM Resource Changed This Pull Request can't be tested by Patchtester PR-4.0-dev labels Feb 2, 2021
@wilsonge
Copy link
Contributor

wilsonge commented Feb 2, 2021

Is there a reason we shouldn't allow people to instantiate their code with jQuery if they specifically include jQuery on the page? Fully understand in core we don't have any reason to

@brianteeman
Copy link
Contributor

Is there a reason we shouldn't allow people to instantiate their code with jQuery if they specifically include jQuery on the page? Fully understand in core we don't have any reason to

Nothing other than "purity"

@wilsonge
Copy link
Contributor

wilsonge commented Feb 2, 2021

Nothing other than "purity"

That's what I want to check. I guess I'm just unsure if you can still use the non-jQuery init if jQuery is actually present on the page.

@Fedik
Copy link
Member

Fedik commented Feb 2, 2021

wait, I have read your doc, I think there something mixed up

do we have a bug with events in the core?
and do you have a non core example when it does not work?

@dgrammatiko
Copy link
Contributor Author

Please read the longer explanation on the link. In sort if bootstrap finds jQuery in the page (and body doesn’t have the data-bs-no-jquery attribute) will automatically switch to jQuery the events. This means that all the code (core or 3rd pd) needs to have a logic to handle both systems... Has nothing to do with purity but having to write the same thing for both vanilla AND jquery is kinda odd

@dgrammatiko
Copy link
Contributor Author

dgrammatiko commented Feb 2, 2021

@Fedik theres nothing mixed up. I tested the code and if jquery with the body attr exists the events are only bubbling in the jquery universe

Also, I'll quote your answer from a similar case: #23062 (comment)

@wilsonge wilsonge merged commit 65c0e60 into joomla:4.0-dev Feb 2, 2021
@Fedik
Copy link
Member

Fedik commented Feb 2, 2021

Please read the longer explanation on the link.

yes, I have read,
that why I asked for an example, I need a real example :)

@wilsonge
Copy link
Contributor

wilsonge commented Feb 2, 2021

Yeah that's unfortunate - but I guess logical. Thanks!

@wilsonge wilsonge added this to the Joomla 4.0 milestone Feb 2, 2021
@Fedik
Copy link
Member

Fedik commented Feb 2, 2021

@wilsonge you a bit to fast 😄

@Fedik
Copy link
Member

Fedik commented Feb 2, 2021

this is not required,
if someone tell me an example with a problem, then I can show you how thing works in bootstrap 5

@wilsonge
Copy link
Contributor

wilsonge commented Feb 2, 2021

@dgrammatiko if you could still give him a "real" example it would be appreciated :) would also be nice as I have to transcribe some of this into the docs to go alongside tonights beta anyhow (I know there's some theoretical stuff in the discussion)

@dgrammatiko
Copy link
Contributor Author

I've preparing something:

@Fedik d/l https://github.com/dgrammatiko/bs5/blob/main/BS5Tests.zip

Replace the contents of tmpl/alerts/default.php to

<?php
/**
 * Bs5test Joomla Component
 *
 * @copyright  Copyright (C) 2021 dGrammatiko. All rights reserved.
 * @license    GNU/GPL - http://www.gnu.org/copyleft/gpl.html
 */
defined('_JEXEC') or die;

use Joomla\CMS\HTML\HTMLHelper;
use Joomla\CMS\Uri\Uri;
use Joomla\CMS\Factory;

include_once __DIR__ . '/../nav.php';

HTMLHelper::_('jquery.framework');
HTMLHelper::_('bootstrap.alert');

Factory::getDocument()->addScriptDeclaration(
		<<<JS
document.addEventListener('DOMContentLoaded', () => {
  // Insert an alert
  document.body.insertAdjacentHTML('beforeend', `<div class="alert alert-info alert-dismissible" role="alert">
  <strong>Holy guacamole!</strong> You should check in on some of those fields below.
  <button type="button" class="btn-close" data-bs-dismiss="alert" aria-label="Close"></button>
</div>`);

  x = document.querySelector('.alert');

  jQuery(x).alert();

  jQuery(x).on('closed.bs.alert', () => {alert('ho ho')})


  document.querySelector(".alert").addEventListener("closed.bs.alert", () => { console.log("YAY"); });
});
JS
);

Also, you need to comment out line 3 // document.body.dataset.bsNoJquery = ''; of the file build/media_source/vendor/bootstrap/js/nojquerymode.es6.js and recompile npm run build:bs5
Check the inconsistency

@Fedik
Copy link
Member

Fedik commented Feb 2, 2021

hm, and where the bug? Everything is working.

Here is simplified version (no need to install component):

In index.php of cassiopeia add $wa->useScript('bootstrap.alert');:
and somewhere in body:

<script type="module">
  if (document.body.hasAttribute('data-bs-no-jquery')) {
    document.body.removeAttribute('data-bs-no-jquery');
  }

  const $alertTest = document.querySelector('.alert-test');

  jQuery($alertTest).alert();

  jQuery($alertTest).on('closed.bs.alert', function (e){
    console.log('jquery event', e);
  });

  $alertTest.addEventListener('closed.bs.alert', function (e){
    console.log('native event', e);
  });
</script>
<div class="alert-test alert alert-info alert-dismissible" role="alert">
	<strong>Holy alert!</strong>
	<button type="button" class="btn-close" data-bs-dismiss="alert" aria-label="Close"></button>
</div>

Then when the home page loaded, try to close the alert.
And look in browser console, you will get 2 events, jquery event and native event.

No bug found 😉

@dgrammatiko
Copy link
Contributor Author

That's half of the test, you need to check also the native constructor new bootstrap.Alert($alertTest);

@Fedik
Copy link
Member

Fedik commented Feb 2, 2021

what wrong with new bootstrap.Alert($alertTest); ? it works

@dgrammatiko
Copy link
Contributor Author

Ok, give me 1 minute then

@dgrammatiko dgrammatiko deleted the 4.0-dev___vanilla-mode branch February 2, 2021 21:47
@Fedik
Copy link
Member

Fedik commented Feb 2, 2021

Some explanation about events, to avoid confusion.

Internally Bootstrap trigger both event types: jQuery (if it on the page) and native:
https://github.com/twbs/bootstrap/blob/14cfb7af93e75e3e03e487bff897edca371d1695/js/src/dom/event-handler.js#L284-L303

What that means?
That means that if your code use jQuery, then you have to use .on()/.off(), and if your code is native javascript then you should use addEventListener().
Mixing is possible, but only if you know what you doing.

@dgrammatiko
Copy link
Contributor Author

Honestly supporting a hybrid system will be a hell for everybody, but whatever I'm reverting this

@dgrammatiko
Copy link
Contributor Author

@Fedik #32256 will allow both Event systems at the same time

@Fedik
Copy link
Member

Fedik commented Feb 2, 2021

Honestly supporting a hybrid system will be a hell for everybody

What exactly do you mean? I do not see that we need to do anything here, it just works.

Forcing data-bs-no-jquery just disable possibility to init bootsrap component via jquery , like jquery(element).blabla();.
That many people use currently.

@dgrammatiko
Copy link
Contributor Author

No problem but when 3rd PD start to have a tonne of problems due to misuse of the listeners I will give them your email 🤣

@Fedik
Copy link
Member

Fedik commented Feb 2, 2021

No problem, I very "polite" in emails 😄

best-regards
:

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

Labels

NPM Resource Changed This Pull Request can't be tested by Patchtester

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants