NPM Resource Changed ? ? Pending

User tests: Successful: Unsuccessful:

avatar dgrammatiko
dgrammatiko
30 Jan 2021

Pull Request for Issue #32185 .

Summary of Changes

The fields Media and User have a dependency on the Bootstrap Modal but the order of the execution is wrong. The modal helper function needs to be called before including the field's own script

Testing Instructions

  • Login to the administrator area
  • Go to Global Configuration and confirm that there are no console errors
  • Edit an article and confirm no console errors and the Created By, Intro Image and Full Article Image fields still working

Actual result BEFORE applying this Pull Request

Expected result AFTER applying this Pull Request

Documentation Changes Required

No

avatar dgrammatiko dgrammatiko - open - 30 Jan 2021
avatar dgrammatiko dgrammatiko - change - 30 Jan 2021
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 30 Jan 2021
Category Layout
avatar dgrammatiko dgrammatiko - change - 30 Jan 2021
Labels Added: ?
avatar dgrammatiko dgrammatiko - change - 30 Jan 2021
The description was changed
avatar dgrammatiko dgrammatiko - edited - 30 Jan 2021
315b16f 30 Jan 2021 avatar dgrammatiko CS
avatar dgrammatiko dgrammatiko - change - 30 Jan 2021
The description was changed
avatar dgrammatiko dgrammatiko - edited - 30 Jan 2021
avatar ceford
ceford - comment - 30 Jan 2021

I did not understand the testing instructions concerning User field in Article edit. However, in Users / Add User Note I get Uncaught ReferenceError: bootstrap is not defined which goes away after applying the patch. But I get this warning in the form: Notice: Undefined variable: uri in /Users/ceford/Sites/joomla-cms-4/layouts/joomla/form/field/user.php on line 61.


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

avatar dgrammatiko
dgrammatiko - comment - 30 Jan 2021

But I get this warning in the form: Notice: Undefined variable: uri in /Users/ceford/Sites/joomla-cms-4/layouts/joomla/form/field/user.php on line 61.

This should be fixed now with bf8f7ec

I did not understand the testing instructions concerning User field in Article edit

Got to the Publishing tab and check the field Created By

avatar dgrammatiko dgrammatiko - change - 30 Jan 2021
The description was changed
avatar dgrammatiko dgrammatiko - edited - 30 Jan 2021
avatar chmst
chmst - comment - 30 Jan 2021

The notice in user field is ok now, but I stll have the error "uncaught.." in media field.

avatar dgrammatiko
dgrammatiko - comment - 30 Jan 2021

The notice in user field is ok now, but I stll have the error "uncaught.." in media field.

What URL?

avatar chmst
chmst - comment - 30 Jan 2021

New article

avatar joomla-cms-bot joomla-cms-bot - change - 30 Jan 2021
Category Layout JavaScript Repository NPM Change Layout
avatar dgrammatiko dgrammatiko - change - 30 Jan 2021
Labels Added: NPM Resource Changed
avatar dgrammatiko
dgrammatiko - comment - 30 Jan 2021

@chmst should be ok after 4355940

avatar Fedik
Fedik - comment - 30 Jan 2021

@dgrammatiko a better fix is to to call Joomla.initialiseModal(this.modalElement, {isJoomla: true });
not in connectedCallback(), but on first interaction, lazylly, and have an internal flag to mark that Joomla.initialiseModal was called.

avatar Fedik
Fedik - comment - 30 Jan 2021

Additionally, instead of callback I would use event:

Joomla.Event.dispatch(this.modalElement, 'joomla:initModal', {isJoomla: true });

So it does not strict to have Joomla.initialiseModal, and anyone can listen to this event.

You see, it useful ?

avatar dgrammatiko
dgrammatiko - comment - 30 Jan 2021

but on first interaction, lazylly, and have an internal flag to mark that Joomla.initialiseModal was called.

True, that a better implementation (btw this PR was done on Github UI, so...)

Additionally, instead of callback I would use event:

In general, I will agree with you here that events are a better way. There are couple caveats here:

  • Bootstrap.Modal itself is a constructor and all the code Joomla.initialiseModal is kinda a wrapper around it
  • The callback is closer to the bootstrap.Modal(element), so I guess easier for Joomla devs (might be wrong here)
  • A way better model is Custom Elements, the constructor is on the actual element so no event no callbacks. The problem is that instead of <bootstrap-modal> we try to apply all the interactivity on a <div>...
avatar Fedik
Fedik - comment - 30 Jan 2021

A way better model is Custom Elements,

Well, it already Custom Element ?

I understood what you mean, however it not really a fix, example when on frondend I want to use FooBarModal.js instead of BotCrap.js.
Event or Callback still better in this case, because I can disable botcrap.js, and hook up my script.

btw this PR was done on Github UI

Not bad as for Github UI ?

avatar chmst chmst - test_item - 30 Jan 2021 - Tested successfully
avatar chmst
chmst - comment - 30 Jan 2021

I have tested this item successfully on 4355940


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

avatar dgrammatiko
dgrammatiko - comment - 30 Jan 2021

I understood what you mean, however it not really a fix, example when on frondend I want to use FooBarModal.js

That's the big problem of HTMLHelper and FormField they need to become client-specific. Actually this is one of the major problems of Joomla and somehow it needs a realistic solution so finally the backend could be decoupled from the frontend.

avatar Fedik
Fedik - comment - 30 Jan 2021

That's the big problem of HTMLHelper

I tried to reduce use of HTMLHelper::_('bootstrap.foobar') (and such) calls to $wa->useScript('bootstrap.foobar'), but I see someone added much more fancy code there ?

But not much important.

avatar dgrammatiko
dgrammatiko - comment - 30 Jan 2021

I see someone added much more fancy code there

If you have a better way then go ahead and change the code there. All I wanted to do there was to modularise the JS, eg include only the JS you need per route (instead of bringing both jQuery and the whole Bootstrap library even if all you wanted was a collapse component).

avatar Fedik
Fedik - comment - 30 Jan 2021

If you have a better way then go ahead and change the code there. All I wanted to do there was to modularise the JS

As I said it is not much important for now.
For now I made this #32204, so later we can improve it, when will have some more time.

avatar dgrammatiko
dgrammatiko - comment - 30 Jan 2021

when will have some more time.

Yeah let's patch only the really breaking or bad things for now and hopefully, we can improve things later on

avatar Fedik Fedik - test_item - 30 Jan 2021 - Tested successfully
avatar Fedik
Fedik - comment - 30 Jan 2021

I have tested this item successfully on 4355940


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

avatar chmst chmst - change - 30 Jan 2021
Status Pending Ready to Commit
avatar chmst
chmst - comment - 30 Jan 2021

RTC


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

avatar wilsonge wilsonge - close - 31 Jan 2021
avatar wilsonge wilsonge - merge - 31 Jan 2021
avatar wilsonge wilsonge - change - 31 Jan 2021
Status Ready to Commit Fixed in Code Base
Closed_Date 0000-00-00 00:00:00 2021-01-31 23:29:21
Closed_By wilsonge
Labels Added: ?
avatar wilsonge
wilsonge - comment - 31 Jan 2021

Thanks!

Add a Comment

Login with GitHub to post a comment