Skip to content

Comments

[4.0] WebAsset for modules#28423

Merged
wilsonge merged 8 commits intojoomla:4.0-devfrom
Fedik:asset-modules
Mar 29, 2020
Merged

[4.0] WebAsset for modules#28423
wilsonge merged 8 commits intojoomla:4.0-devfrom
Fedik:asset-modules

Conversation

@Fedik
Copy link
Member

@Fedik Fedik commented Mar 21, 2020

Summary of Changes

This replaces htmlhelper to webaset for modules

Updated modules:

site/mod_menu
site/mod_languages
site/mod_login
site/mod_wrapper
site/mod_finder
admin/mod_sampledata
admin/mod_quickicon
admin/mod_login
admin/mod_multilangstatus

Components:

site/com_wrapper
site/com_finder 

Testing Instructions

Apply patch, run npm install
Review all updated modules

Expected result

All work

Actual result

All work

Documentation Changes Required

none

ref #22435

@joomla-cms-bot joomla-cms-bot added NPM Resource Changed This Pull Request can't be tested by Patchtester PR-4.0-dev labels Mar 21, 2020
$wa = $app->getDocument()->getWebAssetManager();

// Allow template to provide its own asset for the module
if (!$wa->assetExists('script', 'mod_login.admin'))
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 advantage of this vs a regular template override?

Copy link
Member Author

Choose a reason for hiding this comment

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

Regular template override will override by path, but cannot overide asset by name for modules, because the module will be rendered after index.php of template (I mean template assets loaded before any module).
And example in current case mod_login will replace 'mod_login.admin' to its own (even if the template provide it also).

With suggested changes if template provide 'mod_login.admin' then it will be used instead of one provided by "module"

I found this issue while doing changes for a modules.

I also thinking about something like registerIfNotExists() but not sure currently. For now made extra if() check in modules.

Copy link
Contributor

Choose a reason for hiding this comment

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

But surely that's only useful for presets rather than singular assets like this where we have a single resource? I don't see the benefit here

Copy link
Member Author

Choose a reason for hiding this comment

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

I want allow to template to override asset by "asset name".
To be consistent. Because it possible for components, and not really for modules, because they "goes after" template.

But I can revert such part, not much critical issue.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think let's revert for now please

Copy link
Member Author

Choose a reason for hiding this comment

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

I have reverted,

@wilsonge wilsonge merged commit 1c392c6 into joomla:4.0-dev Mar 29, 2020
@wilsonge
Copy link
Contributor

Thanks!

@wilsonge wilsonge added this to the Joomla 4.0 milestone Mar 29, 2020
@Fedik Fedik deleted the asset-modules branch March 30, 2020 08:15
@Fedik
Copy link
Member Author

Fedik commented Mar 30, 2020

now left layouts (#28431) and hardest part: components 😃

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

NPM Resource Changed This Pull Request can't be tested by Patchtester

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants