[4.0] Change ModuleChromes to JLayouts#23570
Conversation
|
@mbabker I would be interested in your feedback since you also did some thinking on that. 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. |
a235e95 to
fba6545
Compare
|
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 |
|
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.
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. |
|
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. |
I hit that one a lot yesterday. |
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? |
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.
It does work across tabs, but it doesn't work on a tab (fieldset) itself. I just tested. But it's an interesting idea. 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. |
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. |
|
See #23577 for an RFC about parameters |
06a0a4c to
0501adc
Compare
|
Can we get conflicts fixed here please. I'd like to get this in :) |
# Conflicts: # administrator/templates/atum/html/modules.php # administrator/templates/system/html/modules.php # libraries/src/Form/Field/ChromestyleField.php # templates/cassiopeia/html/modules.php # templates/system/html/modules.php
|
Merged and conflicts solved. |
|
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. |
|
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. 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 |
|
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"). |
|
Thanks! This is a nice improvement! |
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
Actual result
Documentation Changes Required
If module chromes are documented somewhere, that needs to be adjusted.