User tests: Successful: Unsuccessful:
Pull Request for Issue #29250
Disable lazyloading on images that have no dimensions or get added on the first page view
none
cc @dgrammatiko given that you wanted to work on that too.
Status | New | ⇒ | Pending |
Category | ⇒ | Layout Libraries Front End Plugins |
It is not about the message it is about the spec for lazyloading that is strongly suggesting using width/height attributes. Without your side can be shifted arround.
Without your site can be shifted arround.
You're wrong, even a simple image without width and height will also push things. So you're disabling a very good implementation without gaining anything. This PR is totally wrong, please close it
PS If the project still wants me to fix this then you have to roll back the plugin for the lazy loaded images because I can't and won't fix that, basically because it's wrong!!!
This is based on a direct recommendation from google having discussed the height and width attributes with them.
direct recommendation
That's a recommendation...
Labels |
Added:
?
|
I can see the logic about the sizes (not that I agree)
The comment about these images being 99% the first things that are being loaded is just plain wrong especially in a blog page
The comment about these images being 99% the first things that are being loaded is just plain wrong especially in a blog page
People are complaining about shifting layouts here when Joomla has several seconds of blank screen and then everything pop up out of nowhere. This is acceptable but images pushing content is unacceptable
The comment about these images being 99% the first things that are being loaded is just plain wrong especially in a blog page
This should be true for the full image right? I have taken off the intro image already.
This pr also ignores that the use of aspect-ratio (https://caniuse.com/#search=aspect-ratio) also prevents the reflow problem.
You can never assume that an article has its image at the top of the page. That might be the case with the default layout (you can change it) and without a big header or modules above the article.
If you really want to deal with this then the only correct way would be to add support for specifying the image dimensions for both intro and full images
The proper solution is around 10 LOC but requires removal of the plugin...
You can never assume that an article has its image at the top of the page. That might be the case with the default layout (you can change it) and without a big header or modules above the article.
yes agree on that this is the reasone we just changed the default layout ;)
The proper solution is around 10 LOC but requires removal of the plugin...
Feel free to do that 10 LOC and remove the plugin when it works better i'm more than happy.
we just changed the default layout ;)
???
???
in this PR i have updated (concerning the full image stuff) just the default layout shipped with the core.
and remove the plugin
That's not on my todo list, I didn't commit the rest of the code when George decided to merge that plugin although I was strongly against it. I won't play this blame game...
That's not on my todo list, I didn't commit the rest of the code when George decided to merge that plugin although I was strongly against it. I won't play this blame game...
Well I don't know the 10 LOC that you proposed. The plugin approache was the first step to move in the right direction. When there are better ways to acomplish the task I'm happy to take a look into that.
???
in this PR i have updated (concerning the full image stuff) just the default layout shipped with the core.
You misunderstood. full_image layout can be included anywhere in the article. By default
<?php echo LayoutHelper::render('joomla.content.full_image', $this->item); ?>
is near the top of the article but there is no reason for it not to be moved to the bottom
The plugin approache was the first step to move in the right direction
The plugin was never the right direction as many people told you in that PR
The plugin was never the right direction as many people told you in that PR
As mention I'm happy to review other solutions. Right before that PR and plugin we had nothing about lazyloading (even shutdown an issue) so it was some kind of first step to have that attribute supported at all. I'm happy to move forward and take the better solutions when we have them.
You misunderstood. full_image layout can be included anywhere in the article.
hmm agree will add back that attribute for now not having that other attributes can be solved outside of this PR.
Category | Layout Libraries Front End Plugins | ⇒ | Libraries Front End Plugins |
I'm happy to move forward and take the better solutions when we have them.
Sorry but that won't be by me I'm out
Sorry but that won't be by me I'm out
Ok, i just mention you as you proposed to work on that in the issue that brian opend.
+1 for removing the plugin. It's neither here nor there. If anything, media manager should be improved to allow attributes when inserting images.
If anything, media manager should be improved to allow attributes when inserting images.
There is nothing that needs to be improved in the media manager!!!
All it takes is to translate my comment #28838 (comment) to code like:
from:
to:
Joomla.editors.instances[editor].replaceSelection(`<img loading="lazy" src="${Joomla.selectedFile.url}" width="${Joomla.selectedFile.width}" height="${Joomla.selectedFile.height}" alt=""/>`);
This PR totally hideous as the lazyload plugin, but keep on doing what you're doing here...
Can you please update the test instructions
Category | Libraries Front End Plugins | ⇒ | JavaScript Repository NPM Change Libraries Front End Plugins |
You need to put px
at the end
and can you please remove the plugin, it's not doing anything useful anymore?
Can you please update the test instructions
Done.
All it takes is to translate my comment #28838 (comment) to code like:
Awesome just commited the change I have also updated the line 41 with a similiar change.
You need to put px at the end
Where do you want me to add that?
width="${Joomla.selectedFile.width}" height="${Joomla.selectedFile.height}"
to
width="${Joomla.selectedFile.width}px" height="${Joomla.selectedFile.height}px"
Labels |
Added:
NPM Resource Changed
|
@zero-24 I provided the proper solution (it's half there, it's not covering all the cases) based on the fact that you will also remove the stupid plugin. Endorsing my code but not removing the plugin is really dishonourable. It's another way of saying "f#$ off" to someone that was very clear many times in this PR that the correct code exists but requires the removal of the useless plugin.
So long, and thanks for the fish....
Endorsing my code but not removing the plugin is really dishonourable. It's another way of saying "f#$ off" to someone that was very clear many times in this PR that the correct code exists but requires the removal of the useless plugin.
I'm sorry it seems you edited the original commet in this thread here? I only got the mention of the px stuff in the mail notification and when i wrote my answear on github.
@zero-24 I provided the proper solution (it's half there, it's not covering all the cases) based on the fact that you will also remove the stupid plugin.
Will take a look into the plugin removal in the comming days, thanks. What exactly is not covered yet (with the plugin removed) that we have to work on to get it right?
Status | Pending | ⇒ | Closed |
Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2020-09-23 16:11:58 |
Closed_By | ⇒ | zero-24 |
DO NOT DISABLE LAZYLOAD!!!
The message is a notification not an error, that simply translates to: the browser didn't found any width/height so it has to calculate those at run time. It's totally fine and much better than no lazy loading. For best performance the width and height need to be set per image.