NPM Resource Changed ? Pending

User tests: Successful: Unsuccessful:

avatar dgrammatiko
dgrammatiko
21 Mar 2021

Pull Request for Issue # .

Summary of Changes

Two more options were added to the assets definition:

  • inline: will inline the source of a js or stylesheet
  • lazy: will render a <link href="path/to/file" rel=print class=js-lazy-loaded data-rel=stylesheet /> but will not handle the lazyloding js part (eg switch the rel value to whatever is in the data-rel )

Testing Instructions

Apply the PR and check the admin login and dashboard pages (the source and the DOM)

Lazy load a stylesheet
Screenshot 2021-03-21 at 16 56 50

Screenshot 2021-03-21 at 16 57 09

Inlining a js script "inlined": true in the joomla.asset.json
Screenshot 2021-03-21 at 16 57 20

Inlining a stylesheet
$wa->registerAndUseStyle('f-a-1', 'administrator/templates/atum/css/vendor/fontawesome-free/fontawesome.css', ['inlined' => true], [], []);

Screenshot 2021-03-21 at 16 57 36

Documentation Changes Required

DO NOT MERGE THIS

@Fedik your input here?

avatar dgrammatiko dgrammatiko - open - 21 Mar 2021
avatar dgrammatiko dgrammatiko - change - 21 Mar 2021
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 21 Mar 2021
Category Administration Templates (admin) Repository NPM Change JavaScript Libraries
avatar dgrammatiko dgrammatiko - change - 21 Mar 2021
The description was changed
avatar dgrammatiko dgrammatiko - edited - 21 Mar 2021
avatar dgrammatiko dgrammatiko - change - 21 Mar 2021
Labels Added: NPM Resource Changed ?
avatar richard67
richard67 - comment - 21 Mar 2021

@dgrammatiko System test is failing at the same place all the time, doesn't look like the usual unrelated issues. Could you check https://ci.joomla.org/joomla/joomla-cms/40988/1/22 ?

avatar joomla-cms-bot joomla-cms-bot - change - 21 Mar 2021
Category Administration Templates (admin) Repository NPM Change JavaScript Libraries Administration Templates (admin) Repository NPM Change JavaScript Installation Libraries
avatar dgrammatiko
dgrammatiko - comment - 21 Mar 2021

@richard67 this is just a working POC. I'm not expecting all changes here to land as a single PR. Basically what matters here are the changes in the stylesRenderer and scriptsRenderer, everything else is just code that bothers me (eg scripts to bottom) or just code to showcase that this is actually working.

avatar joomla-cms-bot joomla-cms-bot - change - 21 Mar 2021
Category Administration Templates (admin) Repository NPM Change JavaScript Libraries Installation Administration Templates (admin) Repository NPM Change JavaScript Libraries
b7da5d9 21 Mar 2021 avatar dgrammatiko tests
691d28e 21 Mar 2021 avatar dgrammatiko undo
dd6b85a 21 Mar 2021 avatar dgrammatiko yay
avatar dgrammatiko
dgrammatiko - comment - 21 Mar 2021

@richard67 tests are passing but people seem against this PR ?

avatar richard67
richard67 - comment - 21 Mar 2021

... but people seem against this PR

@dgrammatiko Thumb down from Sharky and ... from yourself? The latter confuses me a bit.

avatar dgrammatiko
dgrammatiko - comment - 21 Mar 2021

The latter confuses me a bit.

I'm sick and tired of getting a thumb down on every PR I do so I thought I should follow the trend ?

avatar richard67
richard67 - comment - 21 Mar 2021

I'm sick and tired of getting a thumb down on every PR I do so I thought I should follow the trend ?

Ah, then it's ok. I worried a bit you could develop a dissociative identity disorder ?

avatar Fedik
Fedik - comment - 22 Mar 2021

hold on, here is much of everything ?

Please remove inlined option, it a questionable topic, it is better to do separately.
Please revert changes for XHTML, as long as Document may render XML, it should stay.
And revert "conditional", I know it is old, but as long as it not used, it does not hurt to have, and may be still useful for someone. It is better to do separately.

Now about the lazy CSS
I would suggest to do it with help of attributes only, does not need extra option lazy.
We only need to fix that StylesRenderer to accept a custom rel attribute

Check this example:

<link rel="lazy-stylesheet" href="styles.css">

<script type="module">
document.head.querySelectorAll('link[rel="lazy-stylesheet"]').forEach(function($link){
  $link.rel = "stylesheet";
})
</script>

So in the Style asset need to add attribute rel:"lazy-stylesheet", and that all.

I thought we can combine it with data-defer similar to scripts, but now I do not think that it makes sense.
Custom rel attribute should be enough.

avatar dgrammatiko
dgrammatiko - comment - 22 Mar 2021

I would suggest to do it with help of attributes only, does not need extra option lazy.

I like it, so if we agree on this then I will add the code for this (only the lazy-stylesheet) in the other PR.

About the other changes: at some point, we have to discuss what we deliver, conditionals are not supported in any of the Joomla 4 supported browsers and XHTML is dead since 2012 so I have no clue why we have to have those conditionals there.
I did those changes just to (hopefully) open the discussion

About the inlining part, this is something that the CMS should provide as it's easy to code and quite helpful in many instances

In short, I'll take the lazy-stylesheet to the other PR but it would be nice to have some comments on the other issues I raised here

avatar dgrammatiko dgrammatiko - change - 22 Mar 2021
Status Pending Closed
Closed_Date 0000-00-00 00:00:00 2021-03-22 20:35:27
Closed_By dgrammatiko
avatar dgrammatiko dgrammatiko - close - 22 Mar 2021

Add a Comment

Login with GitHub to post a comment