NPM Resource Changed ? Pending

User tests: Successful: Unsuccessful:

avatar dgrammatiko
dgrammatiko
2 Feb 2021

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

avatar dgrammatiko dgrammatiko - open - 2 Feb 2021
avatar dgrammatiko dgrammatiko - change - 2 Feb 2021
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 2 Feb 2021
Category JavaScript Repository NPM Change
avatar dgrammatiko dgrammatiko - change - 2 Feb 2021
Labels Added: NPM Resource Changed ?
avatar wilsonge
wilsonge - comment - 2 Feb 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

avatar brianteeman
brianteeman - comment - 2 Feb 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

Nothing other than "purity"

avatar wilsonge
wilsonge - comment - 2 Feb 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.

avatar Fedik
Fedik - comment - 2 Feb 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?

avatar dgrammatiko
dgrammatiko - comment - 2 Feb 2021

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

avatar dgrammatiko
dgrammatiko - comment - 2 Feb 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)

avatar wilsonge wilsonge - change - 2 Feb 2021
Status Pending Fixed in Code Base
Closed_Date 0000-00-00 00:00:00 2021-02-02 20:12:26
Closed_By wilsonge
avatar wilsonge wilsonge - close - 2 Feb 2021
avatar wilsonge wilsonge - merge - 2 Feb 2021
avatar Fedik
Fedik - comment - 2 Feb 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 :)

avatar wilsonge
wilsonge - comment - 2 Feb 2021

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

avatar Fedik
Fedik - comment - 2 Feb 2021

@wilsonge you a bit to fast ?

avatar Fedik
Fedik - comment - 2 Feb 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

avatar wilsonge
wilsonge - comment - 2 Feb 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)

avatar dgrammatiko
dgrammatiko - comment - 2 Feb 2021

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

avatar Fedik
Fedik - comment - 2 Feb 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 ?

avatar dgrammatiko
dgrammatiko - comment - 2 Feb 2021

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

avatar Fedik
Fedik - comment - 2 Feb 2021

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

avatar dgrammatiko
dgrammatiko - comment - 2 Feb 2021

Ok, give me 1 minute then

avatar Fedik
Fedik - comment - 2 Feb 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.

avatar dgrammatiko
dgrammatiko - comment - 2 Feb 2021

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

avatar dgrammatiko
dgrammatiko - comment - 2 Feb 2021

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

avatar Fedik
Fedik - comment - 2 Feb 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.

avatar dgrammatiko
dgrammatiko - comment - 2 Feb 2021

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 ?

avatar Fedik
Fedik - comment - 2 Feb 2021

No problem, I very "polite" in emails ?

best-regards
:

Add a Comment

Login with GitHub to post a comment