User tests: Successful: Unsuccessful:
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".
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.
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.
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.
If module chromes are documented somewhere, that needs to be adjusted.
Status | New | ⇒ | Pending |
Category | ⇒ | Administration com_modules Templates (admin) Layout Libraries Front End Templates (site) |
Labels |
Added:
?
|
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...
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.
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.
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.
. 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.
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?
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.
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.
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.
Title |
|
Can we get conflicts fixed here please. I'd like to get this in :)
Merged and conflicts solved.
I have tested this item
I have tested this item
Dont' have a way of testing right now - but drone seems very unhappy here - can you check please
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.
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
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) |
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.
Status | Pending | ⇒ | Fixed in Code Base |
Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2019-03-26 16:26:24 |
Closed_By | ⇒ | wilsonge |
Thanks! This is a nice improvement!
@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.