[4.0] spinner as web component #22491

dgrammatiko · 2018-10-04T00:55:21Z

Pull Request for Issue # .

Summary of Changes

Drops the code from the core.js for the spinner (already deprecated in j3) and introduces a web component
Some minor changes in the build module for the web components. The settings.json will now allow custom paths in the media folder. So someone could do some PRs to move the fields custom elements to their appropriate place (/media/system/js/fields/)
Seperated the logic from the mark up for the category field (someone really needs to do this for all the mark up that is hidden in fields or some helpers!!!)
Reworked the custom fields part that injected some inline jQuery script. So now the script is part of the category field (that is the right place for the js to live) and the helper just passes some data to the field. The js is still inline so someone should just move it to a file...
Patched the custom fields to work with the new element
Patched the installation to work with the new element
Patched the com_associations to work with the new element

Testing Instructions

Install Joomla, spinner is ok
Make sure that you have custom fields assigned to one category. Edit an article and change its cat to the one that custom fields are attached. spinner is ok
Try a multilingual and check if the spinner works ok

Expected result

Actual result

Documentation Changes Required

In 4.0 in each page that a spinner is needed the following steps will need to be done:

Include the web component

HTMLHelper::_('webcomponent', 'system/webcomponents/joomla-core-loader.min.js', ['relative' => true, 'version' => 'auto']);

To show the spinner all we need to do is create an element:

spinner = document.createElement('joomla-core-loader');

// Assuming that the spinner is full screen
document.body.appendChild(spinner);

/*
if we need to append the spinner to some other container
all we have to do is replace document.body with the element 
that we want the spinner to appear
*/

To remove the spinner all we need to do is destroy the element:

spinner = document.querySelector('joomla-core-loader');

spinner.parentNode.removeChild(spinner);

if you want to use the same codebase for different versions (j3 and j4) here is a quick code:

if (Joomla.loadingLayer && typeof Joomla.loadingLayer === 'function') {
 // We are in J3 so use the old method
 Joomla.loadingLayer('show');
} else {
 // We are in the future
spinner = document.createElement('joomla-core-loader');
document.body.appendChild(spinner);
}

laoneo · 2018-10-04T05:39:11Z

administrator/components/com_fields/Helper/FieldsHelper.php

-				if (!jQuery(formControl).val() != '" . $assignedCatids . "'){jQuery(formControl).val('" . $assignedCatids . "');}
-			});"
-			);
+			 */


Needs to get removed or?

Yup this inline script cannot be overridden unless you create a plugin (which might not even possible to do in j4 due to name spacing). Asset loading should happen only in layouts

laoneo · 2018-10-04T05:44:32Z

layouts/joomla/form/field/category.php

+// Preload spindle-wheel when we need to submit form due to category selector changed
+\Joomla\CMS\Factory::getDocument()->addScriptDeclaration(
+<<<JS
+document.addEventListener('DOMContentLoaded', function() {


This part should be added only when custom fields are involved

It needs a wrapper similar to the one one top

brianteeman · 2018-10-04T06:26:21Z

build/media/com_associations/js/sidebyside.js

@@ -113,7 +110,7 @@ jQuery(document).ready(function($) {
 			target.setAttribute('data-language', selected.split(':')[0]);

 			// Iframe load start, show Joomla loading layer.
-			Joomla.loadingLayer('show');
+            document.body.appendChild(document.createElement('joomla-core-loader'));


Actually this file needs to be converted to es6 and vanilla js. The indentation won’t solve the real problems here

brianteeman · 2018-10-04T06:26:32Z

build/media/com_associations/js/sidebyside.js

@@ -168,7 +165,8 @@ jQuery(document).ready(function($) {
 		});

 		// Iframe load finished, hide Joomla loading layer.
-		Joomla.loadingLayer('hide');
+        var spinner = document.querySelector('joomla-core-loader');


brianteeman · 2018-10-04T06:26:55Z

build/media/webcomponents/js/core-loader/core-loader.js

+
+            const template = document.createElement('template');
+            template.innerHTML = `<style>{{CSS_CONTENTS_PLACEHOLDER}}</style>
+<div><span class="yellow"></span><span class="red"></span><span class="blue"></span><span class="green"></span><p>&reg;</p></div>`;


This is correctly indented, the contents are part of the template therefore they don’t obey the the cs of the js, something like `<<<JS

` in php world

brianteeman · 2018-10-04T06:28:08Z

installation/template/js/setup.js

@@ -12,7 +12,7 @@
 Joomla.setlanguage = function(form) {
 	var data = Joomla.serialiseForm(form);

-	Joomla.loadingLayer("show");
+    document.body.appendChild(document.createElement('joomla-core-loader'));


brianteeman · 2018-10-04T06:28:23Z

installation/template/js/setup.js

 			} else {
-				Joomla.loadingLayer("hide");
+                var el = document.querySelector('joomla-core-loader')


brianteeman · 2018-10-04T06:59:57Z

There are multiple issues being addressed in this one pr and it really should be one issue per pr. It makes testing much more reliable and avoids parts of the pr not being tested etc

dgrammatiko · 2018-10-04T08:11:34Z

It’s a take it or leave it...

brianteeman · 2018-10-04T08:24:10Z

Thats a shame as its not the right thing to do (as you well know) and its the reason why whe in the past pr's with multiple issues were merged we are wasting time undoing all the mistakes.

Fedik · 2018-10-04T08:35:16Z

maybe can you please skip the category field for now? it will have a huge conflict with #22263

dgrammatiko · 2018-10-04T08:49:38Z

@Fedik unfortuanatelly this is a really poor implementation with so many anti patterns so at some point it needs to be done.

field markup is not overridable
field specific js is loaded in a helper file in com_fields

I know it’s a pain but at some point these needs to be done correctly...

brianteeman · 2018-10-04T09:39:17Z

As I am currently wasting a lot of time redoing a lot of work that was committed in error because it was not tested properly because it was buried in a big pr with other things we must stop accepting any pr that addresses multiple issues at the same time. It is a waste of people's limited time to have to redo things that were not able to be tested!

dgrammatiko · 2018-10-04T09:54:27Z

Ok good luck

joomla-cms-bot added the PR-4.0-dev label Oct 4, 2018

loader

9581e49

laoneo reviewed Oct 4, 2018

View reviewed changes

brianteeman reviewed Oct 4, 2018

View reviewed changes

cs

0edb8d4

dgrammatiko closed this Oct 4, 2018

dgrammatiko deleted the 4.0-dev-loader branch October 4, 2018 09:54

dgrammatiko mentioned this pull request Nov 11, 2018

Custom Elements integration with Custom Fields needs work #23036

Closed

dgrammatiko mentioned this pull request Aug 4, 2019

[4.0] Spinner nth attempt #25776

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[4.0] spinner as web component #22491

[4.0] spinner as web component #22491

dgrammatiko commented Oct 4, 2018

laoneo Oct 4, 2018

dgrammatiko Oct 4, 2018

laoneo Oct 4, 2018

dgrammatiko Oct 4, 2018

brianteeman Oct 4, 2018

dgrammatiko Oct 4, 2018

brianteeman Oct 4, 2018

brianteeman Oct 4, 2018

dgrammatiko Oct 4, 2018

brianteeman Oct 4, 2018

brianteeman Oct 4, 2018

brianteeman commented Oct 4, 2018

dgrammatiko commented Oct 4, 2018

brianteeman commented Oct 4, 2018

Fedik commented Oct 4, 2018

dgrammatiko commented Oct 4, 2018

brianteeman commented Oct 4, 2018

dgrammatiko commented Oct 4, 2018

[4.0] spinner as web component #22491

[4.0] spinner as web component #22491

Conversation

dgrammatiko commented Oct 4, 2018

Summary of Changes

Testing Instructions

Expected result

Actual result

Documentation Changes Required

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brianteeman commented Oct 4, 2018

dgrammatiko commented Oct 4, 2018

brianteeman commented Oct 4, 2018

Fedik commented Oct 4, 2018

dgrammatiko commented Oct 4, 2018

brianteeman commented Oct 4, 2018

dgrammatiko commented Oct 4, 2018