Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[4.0] Pluginless lazyloading for the core #30748

Merged
merged 20 commits into from
Sep 27, 2020

Conversation

zero-24
Copy link
Contributor

@zero-24 zero-24 commented Sep 23, 2020

Pull Request for Issue #30218

Summary of Changes

This is an replacement of #30218 by dropping the core lazyloading plugin as well as implementing higth & width adttributes to the core fields.

Based on @dgrammatiko 's JS code I hope i did not broke anything in that area?

Stoke through parts implemented with PR #30784 .

Testing Instructions

clean install test

upgrade test

  1. On a clean 4.0-dev or 4.0 Beta 4 or latest 4.0 nightly installation, update to the update package built by Drone for this PR. You can find an update package and a custom update URL here: https://ci.joomla.org/artifacts/joomla/joomla-cms/4.0-dev/30748/downloads/35816/.
  2. Verify that the update suceeds without SQL errors.
  3. Check in the list of plugins in backend that the plugin can't be found anymore.

Actual result BEFORE applying this Pull Request

We set the loading attribute without the width and heigth attribute.

Expected result AFTER applying this Pull Request

There is no lazyloading plugin any more but still we set the loading attribute for new images alongside the width and heigth attributes.

Documentation Changes Required

None

Question

Should there be a option to opt-out of automatic lazyloading new images?
I'm not sure whether we might want to document the SQL scripts to remove the lazyloading plugin from the database? @richard67 @wilsonge ?

@dgrammatiko
Copy link
Contributor

@zero-24 the images rendered through the media field still have no width and height but easily fixable:
change


to

editor.value = `${Joomla.selectedFile.url}?width=${Joomla.selectedFile.width}&height=${Joomla.selectedFile.height}`;

Explainer: The value that is stored in the db for each form field media is a url. Urls can have params. We're passing the required data as params, and pick them up in the layout [php: parse_url($url)] and append them to the respected image attributes. FWIW this technic was proposed to solve also the editing of an image (by having a ?version=xxx param) but was never implemented in the new media manager due to the lack of db integration (I think that was planned for the next iteration).

@zero-24
Copy link
Contributor Author

zero-24 commented Sep 23, 2020

Patched thanks @dgrammatiko

@Quy
Copy link
Contributor

Quy commented Sep 23, 2020

Close #30615?

@zero-24
Copy link
Contributor Author

zero-24 commented Sep 23, 2020

Close #30615?

Done

@dgrammatiko
Copy link
Contributor

@zero-24 it seems that I broke something in the media manager at some point. Please add

                                width: this.item.width ? this.item.width : undefined,
                                height: this.item.height? this.item.height : undefined,

after

extension: this.item.extension ? this.item.extension : false,

Also here's a quick implementation of fulltext image layout:

defined('_JEXEC') or die;

use Joomla\Utilities\ArrayHelper;

$params  = $displayData->params;
$images  = json_decode($displayData->images);
$image   = parse_url($images->image_fulltext);
$src     = $image['path'];
$attr    = [];

parse_str($image['query'], $imageParams);

if (count($imageParams))
{
	if ($imageParams['width'] !== 'undefined')
	{
		$attr['width'] = $imageParams['width'] . 'px';
	}

	if ($imageParams['height'] !== 'undefined')
	{
		$attr['height'] = $imageParams['height'] . 'px';
	}
}
?>
<?php if (!empty($images->image_fulltext)) : ?>
	<?php $imgfloat = empty($images->float_fulltext) ? $params->get('float_fulltext') : $images->float_fulltext; ?>
	<figure class="float-<?php echo htmlspecialchars($imgfloat, ENT_COMPAT, 'UTF-8'); ?> item-image">
		<img loading="lazy" src="<?php echo htmlspecialchars($src, ENT_COMPAT, 'UTF-8'); ?>"
			 alt="<?php echo htmlspecialchars($images->image_fulltext_alt, ENT_COMPAT, 'UTF-8'); ?>"
			 itemprop="image"
				<?php echo ArrayHelper::toString($attr); ?>/>
		<?php if (isset($images->image_fulltext_caption) && $images->image_fulltext_caption !== '') : ?>
			<figcaption class="caption"><?php echo htmlspecialchars($images->image_fulltext_caption, ENT_COMPAT, 'UTF-8'); ?></figcaption>
		<?php endif; ?>
	</figure>
<?php endif;

and a screenshot of the expected result
Screenshot 2020-09-23 at 20 26 12

@zero-24
Copy link
Contributor Author

zero-24 commented Sep 23, 2020

Thanks @dgrammatiko i have just implemended the suggested changes for the full and intro image layout as well as the mention JS changes in the media manager. Thanks!

@SharkyKZ
Copy link
Contributor

🤦‍♂️

@zero-24
Copy link
Contributor Author

zero-24 commented Sep 23, 2020

What is wrong @SharkyKZ ?

@SharkyKZ
Copy link
Contributor

Everything. Revert previous commits and don't screw around with image URLs please.

@zero-24
Copy link
Contributor Author

zero-24 commented Sep 26, 2020

Fine for me will do that hopefully tomorrow Thabks for your PR!

@joomla-cms-bot joomla-cms-bot removed the NPM Resource Changed This Pull Request can't be tested by Patchtester label Sep 27, 2020
@zero-24
Copy link
Contributor Author

zero-24 commented Sep 27, 2020

This PR has now been reset to only remove the plugin and add the is_array check for the HTML Helper. Thanks @dgrammatiko 👍

@zero-24 zero-24 added this to the Joomla 4.0 milestone Sep 27, 2020
@dgrammatiko
Copy link
Contributor

I have tested this item ✅ successfully on 8de2081

@zero-24 Thank you for this one!


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/30748.

@Quy
Copy link
Contributor

Quy commented Sep 27, 2020

I have tested this item ✅ successfully on 8de2081


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/30748.

@joomla-cms-bot joomla-cms-bot removed this from the Joomla 4.0 milestone Sep 27, 2020
@Quy
Copy link
Contributor

Quy commented Sep 27, 2020

RTC


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/30748.

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Sep 27, 2020
@richard67
Copy link
Member

richard67 commented Sep 27, 2020

@dgrammatiko @Quy We (Tobias and me) had to add an additional test, see section "upgrade test" in the testing instructions. Could you test that, too? The other test and related code has not been changed, so no need to repeat your previous, valid test for the "clean install test" section. Thanks in advance.

@dgrammatiko
Copy link
Contributor

I have tested this item ✅ successfully on e735059

Update of a clean J4 was fine, plugin was removed successfully


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/30748.

@richard67
Copy link
Member

Update test on MySQLi and PostgreSQL is ok. Now am doing new installation tests for both database types.

@richard67
Copy link
Member

richard67 commented Sep 27, 2020

I have tested this item ✅ successfully on e735059

@HLeithner HLeithner merged commit 9cc6cb1 into joomla:4.0-dev Sep 27, 2020
@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Sep 27, 2020
@HLeithner
Copy link
Member

Thanks for doing it the proper way.

@zero-24 zero-24 deleted the pluginless_lazyloading branch September 27, 2020 20:50
@zero-24
Copy link
Contributor Author

zero-24 commented Sep 27, 2020

Thanks

dgrammatiko added a commit to dgrammatiko/joomla-cms that referenced this pull request Sep 29, 2020
…ger_events_consistency

* '4.0-dev' of github.com:joomla/joomla-cms: (84 commits)
  [4.0] Error in legacy plugins when method contains $event argument (joomla#30575)
  [4.0] Cassiopea table css (joomla#30740)
  [4.0] Blog view links (joomla#30788)
  Change grid minmax definition for newsflash module (joomla#30781)
  [4.0] Pluginless lazyloading for the core (joomla#30748)
  Update package-lock.json (joomla#30713)
  [4.0] mod_article_news readmore (joomla#30780)
  Improve code, remove separator (joomla#30785)
  [4.0] Template layout select (joomla#30772)
  [4.0][CLI] com_finder use console command (joomla#30768)
  [4.0] Modifying com_actionlogs string (joomla#30758)
  [4.0] Fancy selectbox fix (joomla#30739)
  [4.0] Add missing Table Caption (joomla#30763)
  [4.0] Wrap all buttons in btn-group to improve styling (joomla#30761)
  [4.0] Cassiopeia missing string (joomla#30765)
  Improve batch text (joomla#28447)
  Fix icons not displaying (joomla#30749)
  Remove the chrome "cardGrey". The same effect can be achieved by using the module class "card-grey" with the "card" chrome (joomla#30734)
  Remove obsolete html code (joomla#30737)
  [4.0] Check out improvements related to nullable columns (joomla#30747)
  ...

� Conflicts:
�	administrator/components/com_media/resources/scripts/app/Api.js
�	administrator/components/com_media/resources/scripts/components/toolbar/toolbar.vue
�	package-lock.json
dgrammatiko pushed a commit to dgrammatiko/joomla-cms that referenced this pull request Sep 29, 2020
* remove the plugin code

* make sure there is no error message on attributs passed as string

* implement width & height into the media field

* add width & hight to com_media

* add width & height to the file list

* fix file path

* add width & heigth to the media field

* there is no alt text comming from com_media yet

* patch JS thanks @dgrammatiko

* Update installation/sql/postgresql/base.sql

Co-authored-by: Quy <[email protected]>

* Update installation/sql/mysql/base.sql

Co-authored-by: Quy <[email protected]>

* fix issues mention vy @dgrammatiko and implement width and heigth in the intro and full image layout

* When we have no image nothing is going to be displayed

* move json_decode & change back to the original image url

* take off px for width and height

* reset other changes that has been moved to joomla#30784

* also reset the image list layout

* Update libraries/src/HTML/HTMLHelper.php

* Add update SQL scripts 4.0.0-2020-09-27.sql (joomla#47)

Co-authored-by: Quy <[email protected]>
Co-authored-by: Richard Fath <[email protected]>
sakiss pushed a commit to sakiss/joomla-cms that referenced this pull request Oct 16, 2020
* remove the plugin code

* make sure there is no error message on attributs passed as string

* implement width & height into the media field

* add width & hight to com_media

* add width & height to the file list

* fix file path

* add width & heigth to the media field

* there is no alt text comming from com_media yet

* patch JS thanks @dgrammatiko

* Update installation/sql/postgresql/base.sql

Co-authored-by: Quy <[email protected]>

* Update installation/sql/mysql/base.sql

Co-authored-by: Quy <[email protected]>

* fix issues mention vy @dgrammatiko and implement width and heigth in the intro and full image layout

* When we have no image nothing is going to be displayed

* move json_decode & change back to the original image url

* take off px for width and height

* reset other changes that has been moved to joomla#30784

* also reset the image list layout

* Update libraries/src/HTML/HTMLHelper.php

* Add update SQL scripts 4.0.0-2020-09-27.sql (joomla#47)

Co-authored-by: Quy <[email protected]>
Co-authored-by: Richard Fath <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Language Change This is for Translators
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants