? Pending

User tests: Successful: Unsuccessful:

avatar dgt41
dgt41
4 Mar 2018

Joomla 3.x is extremely tightly coupled to Bootstrap, let's fix this, step by step

Summary of Changes

  • Introduce the API for modals:
Joomla.Modal = {
	/**
	 * *****************************************************************
	 * Modals should implement
	 * *****************************************************************
	 *
	 * getCurrent  Type  Function  Should return the modal element
	 * setCurrent  Type  Function  Should set the modal element
	 * current     Type  {node}    The modal element
	 *
	 * USAGE (assuming that exampleId is the modal id)
	 *   To get the current modal element:
	 *      Joomla.Modal.current; // Returns node element, eg: document.getElementById('exampleId')
	 *   To set the current modal element:
	 *      Joomla.Modal.setCurrent(document.getElementById('exampleId'));
	 *
	 * *************************************************************
	 * Joomla's UI modal uses `element.close();` to close the modal
	 * and `element.open();` to open the modal
	 * If you are using another modal make sure the same
	 * functionality is bound to the modal element
	 * @see media/legacy/bootstrap.init.js
	 * *************************************************************
	 */
        current = '',
	getCurrent: function() {
		return this.current;
	},
	setCurrent: function(element) {
		this.current = element;
	}
};
  • remove jQuery dependency form custom elements: joomla-field-media and joomla-field-user
  • replace all (probably there are some more) instances of jQuery('bla').modal('hide') and jQuery('bla').modal('show')

Why?

  • Joomla is using (esp in the admin area) too many modals with IFRAMES and reverting this to another way means way too deep architectural changes. So this PR is introducing an API, to standardise the modal main functions (open and close). The nice part is that inside an IFRAME you can cll the parent window Joomla object and get the id of the modal and then close it without either hardcoding the id, passing the id to the iframe, using a standard command that will work with J4's custom element UI or Bootstrap's js modal component. Actually, if any modal is implemented correctly (using the API) the commands will work with anything out in the wild (material, semantic-ui, foundation, you name it). And that is (js) decoupling...

Testing Instructions

Install a fresh j4 from this branch.
Insert some sample data.
Go to new article-> tab Publishing and open the Created By modal.
Open your browser's console and type:

Joomla.Modal.current.close()

and then press enter!

voilà

Check also the media field (intro and full article images)

Expected result

Everything should work as before, this is a change that affects (in a positive way) developers not the end users

Actual result

Documentation Changes Required

YES. The API should be well documented!

In essence this is a replacement of the jModalClose() that exists in J3, bound to Mootools modal and hacked to also (kinda) work with Bootstrap. So even if this is not approved we still need something to replace the jModalClose()...

@wilsonge @mbabker @dneukirchen @Fedik let's decide what we should do here so @C-Lodder and @ciar4n have a clear path for the templates!

avatar dgt41 dgt41 - open - 4 Mar 2018
avatar dgt41 dgt41 - change - 4 Mar 2018
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 4 Mar 2018
Category Administration com_plugins com_redirect com_users JavaScript Repository Layout
avatar dgt41 dgt41 - change - 4 Mar 2018
Labels Added: ? ?
avatar dgt41 dgt41 - change - 4 Mar 2018
The description was changed
avatar dgt41 dgt41 - edited - 4 Mar 2018
avatar dgt41 dgt41 - change - 4 Mar 2018
The description was changed
avatar dgt41 dgt41 - edited - 4 Mar 2018
avatar dgt41 dgt41 - change - 4 Mar 2018
The description was changed
avatar dgt41 dgt41 - edited - 4 Mar 2018
avatar dgt41 dgt41 - change - 4 Mar 2018
The description was changed
avatar dgt41 dgt41 - edited - 4 Mar 2018
avatar dgt41 dgt41 - change - 4 Mar 2018
The description was changed
avatar dgt41 dgt41 - edited - 4 Mar 2018
avatar joomla-cms-bot joomla-cms-bot - change - 4 Mar 2018
Category Administration com_plugins com_redirect com_users JavaScript Repository Layout Administration com_banners com_fields com_plugins com_redirect com_users JavaScript Repository Layout
avatar Fedik
Fedik - comment - 8 Mar 2018

okay, looks good but still not perfect :neckbeard:

Personally I would prefer something "event based" without these 1000 callbacks (which still can be missed at some point of time).

Something like:

// Parent page
$(document).on('show.bs.modal', function(event) {
   Joomla.Event.listenOnce(event.target.iframeWindowContent, 'joomla:modal:doClose', function(){
      event.target.close();
   })
});

// Modal Iframe document
<button onclick="Joomla.Event.dispatch('joomla:modal:doClose')">close</button>

In 2 words: when user click close (or the script do request for close) it will dispatch custom event joomla:modal:doClose against it's window object (iframe window), this event will be catched by parent page, and in result the "parent code" will close the modal.

Profit here: you do not have to guess whether window.parent.Joomla.blabla even exists.
(btw, similar approach would be good as replacement for window.parent.JUserSelect(), jSelectArticle() and for other similar trap)

but

If we go the callback way Joomla.currentModal.get().close(), then we need small improve here.
as @dneukirchen already wrote in Glip.

It better to have Joomla.Modal object, which can be easily extended in future.

Joomla.Modal = {
  current: null,
  getCurrent: function() { return this.current; }
  setCurrent: function(element) { this.current = element; }
};

(btw, to support "multi modals" current can be transformed to array of opened modals and Joomla.Modal.getCurrent() will return only last item from array.)

Other notes, 2 potential issue here:

  1. the code will fail if modal initialized somewhere externally, I mean without:
element.open = function () {
   return $self.modal('show');
};
  1. modal element (not bootstrap) has own .open()/.close() methods, wich doing something else ?
avatar dgt41
dgt41 - comment - 9 Mar 2018

the code will fail if modal initialized somewhere externally, I mean without element.open()

True, but we are creating an API so we can then say: use the API properly ?

modal element (not bootstrap) has own .open()/.close() methods, wich doing something else

Where did I saw that naming problem recently? Oh yeah in Javascript itself... Anyways the open/close can be named as jOpen/jClose (although I still like the open close, and letting people the don't want to use the Joomla's core UI components to figure out that themselves...

Now about the event driven approach: personally I don't mind but for newcomers it's harder to grasp the idea and the whole mechanics behind it. So depending what's our target audience (and I think that most people using/coding for joomla are not fluent in js) we should pick the best approach. If we can document this is=n plain english (that won't be me obviously) then we can go this way as well (and the dropping of the window.parent is a very tempting reason to follow this approach).

avatar dgrammatiko dgrammatiko - change - 9 Apr 2018
The description was changed
avatar dgrammatiko dgrammatiko - edited - 9 Apr 2018
avatar dgrammatiko
dgrammatiko - comment - 9 Apr 2018

@dneukirchen so I updated this one to the specs we discussed in Koln.
@Fedik what do you think about this?

avatar dgrammatiko dgrammatiko - change - 9 Apr 2018
Title
[4.0][RFC][POC] Introduce modals API
[4.0]Introduce modals API
avatar dgrammatiko dgrammatiko - edited - 9 Apr 2018
avatar Fedik
Fedik - comment - 9 Apr 2018

@dgrammatiko looks good, except 2 thing :neckbeard:

  1. you still have the code which use an old approach Joomla.currentModal.get().close()
  2. you use Joomla.Modal.setCurrent(element), but then you access directly to the .current property Joomla.Modal.current.close(). This looks inconsistent.
avatar dgrammatiko dgrammatiko - change - 9 Apr 2018
Labels Removed: ?
avatar dgrammatiko dgrammatiko - change - 9 Apr 2018
The description was changed
avatar dgrammatiko dgrammatiko - edited - 9 Apr 2018
avatar dgrammatiko
dgrammatiko - comment - 9 Apr 2018

you still have the code which use an old approach Joomla.currentModal.get().close()

Thanks, I missed those

but then you access directly to the .current property Joomla.Modal.current.close(). This looks inconsistent.

I thought that was what David wanted, I also like the idea of setters and getters as they are more explanatory, but I m also ok with this ?‍♂️

avatar Fedik
Fedik - comment - 9 Apr 2018

I think @dneukirchen meant to use Joomla.Modal instead of Joomla.currentModal,
but better ask him ?

With getters is better, because we can have more control over it,
example can trow an Error with explanation message, when user call getCurrent() before setCurrent()

avatar dneukirchen
dneukirchen - comment - 9 Apr 2018

I think @dneukirchen meant to use Joomla.Modal instead of Joomla.currentModal,
but better ask him ?

Yes, but thats only about naming.

avatar dgrammatiko
dgrammatiko - comment - 9 Apr 2018

Do you want me to use the getter instead of directly calling the stored value?

avatar dgrammatiko
dgrammatiko - comment - 20 May 2018

@Fedik @dneukirchen I made this codepen: https://codepen.io/dgrammatiko/pen/bMzpqB

screen shot 2018-05-20 at 16 29 39
screen shot 2018-05-20 at 16 30 01

So do we agree on correcting this from Joomla.Modal.current to Joomla.Modal.getCurrent() in combination to the exposed properties on the modals (onOpen and onClose)?

avatar Fedik
Fedik - comment - 21 May 2018

looks good,

do we agree on correcting this from Joomla.Modal.current to Joomla.Modal.getCurrent()

yes I think use Joomla.Modal.getCurrent() is better

avatar joomla-cms-bot joomla-cms-bot - change - 21 May 2018
Category Administration com_plugins com_redirect com_users JavaScript Repository Layout com_banners com_fields Administration com_banners com_fields com_redirect com_users JavaScript Repository Layout
4dd81fc 3 Jun 2018 avatar dgrammatiko oops
ba00a30 3 Jun 2018 avatar dgrammatiko 🐶
728d470 3 Jun 2018 avatar dgrammatiko cs
3d4b76e 3 Jun 2018 avatar dgrammatiko cs
20ca491 3 Jun 2018 avatar dgrammatiko cs
7254db9 3 Jun 2018 avatar dgrammatiko cs
a536c4a 3 Jun 2018 avatar dgrammatiko cs
15c100b 3 Jun 2018 avatar dgrammatiko cs
dce71dd 3 Jun 2018 avatar dgrammatiko oops
avatar laoneo laoneo - change - 3 Jun 2018
Status Pending Fixed in Code Base
Closed_Date 0000-00-00 00:00:00 2018-06-03 13:34:30
Closed_By laoneo
avatar laoneo laoneo - close - 3 Jun 2018
avatar laoneo laoneo - merge - 3 Jun 2018
avatar laoneo
laoneo - comment - 3 Jun 2018

Merged to continue the JS API.

avatar dgrammatiko
dgrammatiko - comment - 3 Jun 2018

Thank you @laoneo

Add a Comment

Login with GitHub to post a comment