Skip to content

[4.0] modal fixes for tinyMCE#15894

Closed
dgrammatiko wants to merge 4 commits intojoomla:4.0-devfrom
dgrammatiko:#####4.0-modals
Closed

[4.0] modal fixes for tinyMCE#15894
dgrammatiko wants to merge 4 commits intojoomla:4.0-devfrom
dgrammatiko:#####4.0-modals

Conversation

@dgrammatiko
Copy link
Contributor

Pull Request for Issue # .

Summary of Changes

  • use the modal width, height in tinyMCE
  • jModalClose becomes Joomla.UI.Modal.close
  • fixes all xtd-buttons to the new API point
  • Drop IE8 from drag and drop plugin
  • remove mediafield.*.js from media/media the correct path is system/fields

Testing Instructions

check if all the xtd buttons work in tinyMCE

Expected result

screen shot 2017-05-08 at 17 59 41

Actual result

Documentation Changes Required

});

// API used to be jMoadalClose
if(!Joomla.UI.Modal.close) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@wilsonge @mbabker is this ok or not something we should touch?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the change? Why would this not be defined?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me try to explain:
J3 had this:

		// Attach modal behavior to document
		$document
			->addScriptDeclaration(
			"
		jQuery(function($) {
			SqueezeBox.initialize(" . $options . ");
			SqueezeBox.assign($('" . $selector . "').get(), {
				parse: 'rel'
			});
		});
		function jModalClose() {
			SqueezeBox.close();
		}"
		);

and we were using jModalClose as an API point for modal close (only real use was in tinyMCE).
This code doesn't exist anymore (mootools) so we either have to reintroduce a global function jModalClose or use the Joomla object for that. Obviously I'm in favour of using the Joomla object everywhere... (we can still patch this for B/C, if that is a concern)

Also if we decide to go the custom elements sometime we can have something like Joomla.BS.modal for bootstrap and Joomla.UI.modal for the core, I hope you're getting the easy bridge there and how such a thing could be used to support natively whatever framework...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm happy with the Joomla object. Adding a deprecated jModalClose that proxies to Joomla.UI sounds like a good idea though?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have no clue how extensive was the use of jModalClose by devs.
Also it won't hurt (performance wise) if we just declare window.jModalClose = Joomla.UI.Modal.close

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm down with that

@dgrammatiko
Copy link
Contributor Author

@wilsonge here is the full version of this idea:

// An object to hold UI related methods and events
Joomla = {};
Joomla.UI = {
	methods: {
		Modal: {
			close: function(element) {
				for (var p in Joomla.UI) {
					if (p !== 'methods' && p !== 'events') {
						if (Joomla['UI'][p]['Modal'] && Joomla['UI'][p]['Modal']['close']) {
							Joomla['UI'][p]['Modal']['close'](element ? element : null);
						}
					}
				}
			},
			open: {
				// Propagate the call to all frameworks
				// Framework initiator has the responsibility to
				// Either respond and execute or do nothing
			}
		}
	},
	events: {
		Modal: {
			onShow: {

			}
		}
	}
};
Joomla.UI.Joomla = {
	Modal: {
		close: function(element) {
			// If there is a modal with the given element close it,
			// else check if ther is a BS modal open and then close that
			// else just do nothing
			console.log('I will close the Joomla custom elements modal');
			return false;
		}
	}
};

Joomla.UI.BS = {
	Modal: {
		close: function(element) {
			console.log('I will close the BS modal');
			return false;
		}
	}
};

Joomla.UI.Foundation = {
	Modal: {
		close: function(element) {
			console.log('I will close the Foundation modal');
			return false;
		}
	}
};

Copy paste it in your browsers terminal press return and then enter Joomla.UI.methods.Modal.close()

Pretty straight forward (especially if Joomla cover Bootstrap and custom Elements)

@wilsonge
Copy link
Contributor

wilsonge commented May 8, 2017

What's Joomla.UI.Joomla for - it's not quite obvious

@dgrammatiko
Copy link
Contributor Author

Dummy name (maybe for custom elements?)

@dgrammatiko
Copy link
Contributor Author

Think Joomla, BS and Foundation as different concurrent running frameworks

@wilsonge
Copy link
Contributor

wilsonge commented May 8, 2017

I'm unsure then. I'm very unsure about making it 'easy' for people to support different frontend frameworks at the same time. It's something that's just asking for a disaster to happen. If we wanted to support that then your structure makes sense. But we actively do not want to support that.

@dgrammatiko
Copy link
Contributor Author

The point is not to have multiple concurrently running frameworks! That would be plain stupid, and anyone doing that should be banned from the internet :) .
Essentially this is about the decoupling from any framework and defining an API that will not change in the next version of bootstrap or whatever the future holds for the project. Maybe I gave you the worst example ever, here :(

@Fedik
Copy link
Member

Fedik commented May 9, 2017

hm, I am more and more thinking that the system modals (select file, select article, select user etc etc) should be a standalone script, without relating to an any framework, then we will not have such problem at all. As it was in the past.

Really it just make a problems, where they should not be

upd: In my extensions I use http://dimsemenov.com/plugins/magnific-popup/ but in the wild should be something without jQuery for J4.0 for sure

@dgrammatiko
Copy link
Contributor Author

@Fedik my idea was to use custom elements for whatever Joomla is using (tabs, modals, accordion, dropdown), and here is my first attempt: https://dgt41.github.io/bs4-custom-elements/
Good thing with this approach is that we go framework free...

@Fedik
Copy link
Member

Fedik commented May 9, 2017

@DGT41 maybe I something not understood,
Let's say, Joomla! will have <bs4-modal>blablabl</bs4-modal>
and I am doing the template on my site, based on Foundation. what should I do?
how I should use <bs4-modal></bs4-modal>?

@dgrammatiko
Copy link
Contributor Author

dgrammatiko commented May 9, 2017

<bs4-modal></bs4-modal> is a custom element that comes with predefined methods, events, attributes and of course it's own styles. So by including (e.g. $doc->addCustomElement('bs4-modal')) you are already CSS framework free and javascript framework free! This is how a div or an input is build inside any browser so instead of cannibalising some divs to make a modal we are defining it. That's the way to go for UI, IMHO

@Fedik
Copy link
Member

Fedik commented May 9, 2017

thanks for explanation, this is still confusing 😉

@dgrammatiko
Copy link
Contributor Author

Maybe looking at some other custom elements is more explanatory, http://strand.mediamath.com/strand-dialog.html

@dgrammatiko
Copy link
Contributor Author

But we actively do not want to support that

@wilsonge you realise that the code I used in the comments Joomla.UI.methods.Modal.close() is not the same as the code used in the PR Joomla.UI.Modal.close(), so let me know how you want to resolve this.

@Fedik you mention that you have an idea for the modal close mess, can you give us an example?

@Fedik
Copy link
Member

Fedik commented May 15, 2017

@DGT41 yes, I want to suggest another idea, based on custom events.
The key is that "script which open the modal should close it, by himself, and do not wait when someone will do it"

As I have mentioned in #16016, the custom events can be used to send a "data" from script to script. So we can use it for the modal, to send data from the child window to the parent.

Pseudo code, for select article modal:

// field-modal js
(function(){

 // Open popup. UPD: Open programmaticaly
 openModal(); 
 // Wait when user do the chose
 iframe.window.addEventListener('joomla:articleSelected', function(event) {
   var artIcleId = event.details.id,
         artTitle   = event.details.title;

    // Close popup window
   closeModal();
 });

})();
<!--  Article list -->
<a href="#" onclick="Joomla.Event.dispatch('joomla:articleSelected', {id: 100, title: 'Free beer!'})">
  Select This article</a>

So, in final the "Article list" do not care who open the window, it just dispatch joomla:articleSelected (or something) when user made chose, and the field-modal catch it and close the window itself.

@dgrammatiko
Copy link
Contributor Author

@Fedik the modal can be either bootstrap or tinymce, how are we dealing with that case?

@Fedik
Copy link
Member

Fedik commented May 15, 2017

@DGT41 ah, I forgot that the bootstrap modal opens "magicaly" 😄
I think that for complex cases we should open them "programmaticaly" only,
see "openModal()" in my pseudo code? 😉

@Fedik
Copy link
Member

Fedik commented May 15, 2017

then you do not need to do stuff like:

window.parent.Joomla.editors.instances[editor].replaceSelection(tag);
window.parent.Joomla.UI.Modal.close();

which is "not nice" to be honest 😄

@Fedik
Copy link
Member

Fedik commented May 15, 2017

and bonus, you do no need to know the editor id! 🎉

@dgrammatiko
Copy link
Contributor Author

Same view is used in:
xtd button,
tinyMCE button,
admin com_menu (select article button)

If all these cases will work with the proposed code (event driven) let's do that then

@dgrammatiko
Copy link
Contributor Author

Too many things in one PR, will split this to something easier to test/review

@dgrammatiko dgrammatiko deleted the #####4.0-modals branch July 22, 2017 20:12
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.

4 participants