? Pending

User tests: Successful: Unsuccessful:

avatar Bakual
Bakual
16 Jan 2019

Inspired on some ideas from Michael in #20864 and some recent discussions about the Module Class behavior.
For those who aren't aware: The Module Chrome is the part around the actual module output. In the module settings, it is called "Module Style".

Summary of Changes

This PR moves the module chromes (the code in that funny /templates/yourtemplate/html/modules.php file) to JLayouts.
The ones from the system templates are in the global /layouts/chromes/ folder.
The ones from the core templates are in the respective templates html/layouts/chromes/ folder.

Backward compatibility related

Support for chromes defined in modules.php is removed. Tenplates who wish to support both J3 and J4 can do so by adding the chromes as JLayouts and still keep the modules.php. In J3, the legacy one is used and in J4 the JLayouts are used.

The protected function Joomla\CMS\Form\Field\ChromestyleField::getSystemTemplate() is removed since it no longer is used.

Existing settings in modules for the module style keep working as the value stored in J3 and J4 is exactly the same.

Testing Instructions

Set different module styles and test if the module is rendered with the expected chrome.
Test if you can add new layouts or override the existing chromes.

Expected result

  • Works from the JLayouts
  • Custom chromes are possible
  • Chromes are overrideable

Actual result

  • Works, but uses modules.php
  • No customisation possible

Documentation Changes Required

If module chromes are documented somewhere, that needs to be adjusted.

avatar Bakual Bakual - open - 16 Jan 2019
avatar Bakual Bakual - change - 16 Jan 2019
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 16 Jan 2019
Category Administration com_modules Templates (admin) Layout Libraries Front End Templates (site)
avatar Bakual
Bakual - comment - 16 Jan 2019

@mbabker I would be interested in your feedback since you also did some thinking on that.
@ciar4n Would love some feedback from you as well.

If that is approved, I'm thinking further to allow a XML file along the JLayout which would allow to define chrome specific parameters. But that will be a second PR and needs a bit more thoughts I guess.

avatar Bakual Bakual - change - 16 Jan 2019
Labels Added: ?
avatar Hackwar
Hackwar - comment - 16 Jan 2019

We indeed have to modernise the module chrome and 4.0 would be a good place to do this. However I would prefer if we would go a different route. I do kinda like that the chrome is not yet another anonymous file that no IDE knows how to treat, but a function (or method) that has a defined scope.

Thus I would propose to have a Joomla\CMS\Module\ModuleChrome class that implements a getChromeNames() method to have a list of available chromes in the current class and then a template specific class in a module.php or chrome.php in every template. That template class could extend from the system templates module chrome class which has a bunch of default chromes. The chromes are then just arbitrarily named methods of that class and the above mentioned method returns just the names of the functions that you can call. The ModuleChrome class could simply diff the methods of the base class and the inheriting class, while templates could implement their own code in case that isn't enough.

This idea is more based on an uneasy feeling about creating a file for every chrome with all the overhead in the files, the difficulties for the IDEs and the overhead by using JLayout. I'm absolutely no fan of JLayout... But the simple fact that no IDE will be able to decipher the current scope of a JLayout file and all the copyright headers and merging variables into the current scope and all that, make me want to have a more object oriented approach. A chrome like none would be public function none($module) {return $module->text;} or something like that instead of the 40 lines that would be the prolog in every JLayout file...

avatar Bakual
Bakual - comment - 16 Jan 2019

It's true that JLayouts do not play well with IDEs. That's a general issues with any JLayout and tmpl file because they are layout files that are just included and not PHP classes.
I still decided to use JLayouts because it's the way we render HTML almost everywhere in Joomla. I don't think it's wise to use yet another complex mechanism to render a few HTML tags.
JLayouts also allow the user to easily add or edit existing ones. They can even use chromes from templates which aren't active (of course CSS is missing then). If they know how JLayouts work, they don't have to learn anything on top.
It offers a lot of flexibility without having to add any line of code, it just comes by using JLayouts. That's a big win in my eyes.

A chrome like none would be public function none($module) {return $module->text;} or something like that instead of the 40 lines that would be the prolog in every JLayout file...

The "none" chrome has exactly 12 lines of code: https://github.com/joomla/joomla-cms/blob/fba65450da293e6f5e4f49ce117ba45a05227575/layouts/chromes/none.php. 10 lines of those are the "header", that's true, but your "none" function would need some PHPDoc as well, so you don't save all of those 10 lines.
The biggest one is the "well" one in Atum with 52 lines. That one already is quite complex in fact.

avatar ciar4n
ciar4n - comment - 17 Jan 2019

This makes a lot of sense. As you say, it brings chromes inline with how most other HTML is rendered. Although I've gotten used to it, initially the modules.php seemed detached to how HTML was generally rendered within Joomla.

Been overridable gives plenty of obvious gains. From a template perspective, it allows changes to default chromes that users would be using (known or not). A module set as a tab for example (if one was to exist) could keep the same chrome settings between templates as the template would override that default chrome to match their own framework/personal code. Currently new chromes have to be created even thou differences may be subtle to the default. Considering that at some point it might be good to relook at the default chromes which in some cases are outdated and also how they are described. As Micheal pointed out in the original issue, the current method of selecting chromes is far from descriptive.

avatar Bakual
Bakual - comment - 17 Jan 2019

One thing I noticed is that the chrome jlayouts are not listed in the template override manager. That is because I've stored them in /layouts/chromes and the manager expects a folder more, eg /layouts/chromes/system.
Overrides work perfectly fine the way I currently did it.
Imho that limitation of the override manager is a bit stupid. Also the fact that it only creates overrides for the whole folder and not for a single file isn't that nice. So I'd rather see the override manager improved to be based on files, not folders which would fix this issue.

avatar brianteeman
brianteeman - comment - 17 Jan 2019

. Also the fact that it only creates overrides for the whole folder and not for a single file isn't that nice.

I hit that one a lot yesterday.

avatar ciar4n
ciar4n - comment - 17 Jan 2019

If that is approved, I'm thinking further to allow a XML file along the JLayout which would allow to define chrome specific parameters. But that will be a second PR and needs a bit more thoughts I guess.

I guess the XML loaded would have to change dynamically depending on the 'Module Style' selected which AFAIK would be new territory for the UI. Loading each XML into separate tabs and then using 'showon'? Does 'showon' work on tabs?

avatar Bakual
Bakual - comment - 17 Jan 2019

I guess the XML loaded would have to change dynamically depending on the 'Module Style' selected which AFAIK would be new territory for the UI

This approach wouldn't be new. We use it for example for custom fields. When we change the category the whole form is reloaded because the custom fields could be different. But it's not what I want to do here.

Loading each XML into separate tabs and then using 'showon'? Does 'showon' work on tabs?

It does work across tabs, but it doesn't work on a tab (fieldset) itself. I just tested. But it's an interesting idea.
What I already have in another branch is that you can add an XML into the chromes folder where you define a field like this:

<field
		name="cassiopeia_test"
		type="text"
		label="Test Field Cassiopeia"
		showon="style:Cassiopeia-cardGrey"
/>

It gets loaded and only shows if the "cardGrey" chrome is selected. I prefixed the param name with the template name so it certainly doesn't clash with other parameters.

avatar ciar4n
ciar4n - comment - 17 Jan 2019

This approach wouldn't be new. We use it for example for custom fields.

Of course... I'd forgotten about that.

In using 'showon' we would be relying on template devs to set this value in their custom XML which I guess would not be ideal. Otherwise, such settings will be displayed regardless of the 'Module Style' value. Not sure if there is a way around that.

avatar Bakual
Bakual - comment - 17 Jan 2019

I'd prefer to stop the discussion around parameters here to not get more off-topic.
I can add a second PR for that as I expect more discussions for it.

avatar Bakual Bakual - change - 17 Jan 2019
Title
Change ModuleChromes to JLayouts
[4.0] Change ModuleChromes to JLayouts
avatar Bakual Bakual - edited - 17 Jan 2019
avatar Bakual
Bakual - comment - 17 Jan 2019

See #23577 for an RFC about parameters

0501adc 22 Jan 2019 avatar Quy CS
avatar wilsonge
wilsonge - comment - 21 Mar 2019

Can we get conflicts fixed here please. I'd like to get this in :)

avatar Bakual
Bakual - comment - 21 Mar 2019

Merged and conflicts solved.

avatar alikon
alikon - comment - 22 Mar 2019

I have tested this item successfully on 9cfcc76


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

avatar alikon
alikon - comment - 22 Mar 2019

I have tested this item successfully on 9cfcc76


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

avatar alikon alikon - test_item - 22 Mar 2019 - Tested successfully
avatar wilsonge
wilsonge - comment - 22 Mar 2019

Dont' have a way of testing right now - but drone seems very unhappy here - can you check please

avatar Bakual
Bakual - comment - 22 Mar 2019

Drone is currently unhappy in the base branch and this PR was based on that "unhappy" branch. So I'd expect nothing else.
Check maintainer glip chat for details

Updating the branch once the base branch is fixed should solve it. ?

avatar wilsonge
wilsonge - comment - 22 Mar 2019

I already did merge it in. And restarted drone like 4 times. So feel like it's a legit failure. Because it very rarely needs more than 2

avatar joomla-cms-bot joomla-cms-bot - change - 22 Mar 2019
Category Administration com_modules Templates (admin) Layout Libraries Front End Templates (site) Administration com_login com_modules Templates (admin) Layout Libraries Front End Templates (site)
avatar Bakual
Bakual - comment - 22 Mar 2019

Ah, didn't see you already merged the branch. When I rebased the PR I saw it failing but already before the system tests even started. Now that this Javascript issue is solved, I saw the system test failure.

I now found the issue. It's actually because the login page requested a non-existing chrome ("rounded").
I've now added a fallback in case the chrome is missing plus removed the reference to the non-existing chrome.

avatar wilsonge wilsonge - change - 26 Mar 2019
Status Pending Fixed in Code Base
Closed_Date 0000-00-00 00:00:00 2019-03-26 16:26:24
Closed_By wilsonge
avatar wilsonge wilsonge - close - 26 Mar 2019
avatar wilsonge wilsonge - merge - 26 Mar 2019
avatar wilsonge
wilsonge - comment - 26 Mar 2019

Thanks! This is a nice improvement!

Add a Comment

Login with GitHub to post a comment