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

Lazyload for CSS background images - do not modify excluded images markup #6146

Closed
viobru opened this issue Sep 6, 2023 · 2 comments · Fixed by #6203
Closed

Lazyload for CSS background images - do not modify excluded images markup #6146

viobru opened this issue Sep 6, 2023 · 2 comments · Fixed by #6203
Assignees
Labels
effort: [S] 1-2 days of estimated development time module: lazyload background css images priority: medium Issues which are important, but no one will go out of business. type: enhancement Improvements that slightly enhance existing functionality and are fast to implement
Milestone

Comments

@viobru
Copy link
Contributor

viobru commented Sep 6, 2023

Is your feature request related to a problem? Please describe.
Currently, when an image is excluded from the feature, the data-rocket-lazy-bg="loaded" is still added to it, and the background-image value is still switched to a variable, like if the feature was applied to it, although the excluded image is indeed loaded without lazyload.

Describe the solution you'd like
Excluded images could have the attribute not added and the background-image value not changed; i.e. the excluded images markup could not be modified to prevent confusion.

Describe alternatives you've considered
If applying all these changes is too complex, we should document this properly informing the customers that all this will still be applied, but that they can check in the network tab that excluded images are indeed excluded.

Additional context
Screenshots:
https://share.getcloudapp.com/RBuzexNE
https://share.getcloudapp.com/RBuzexrl

@piotrbak
Copy link
Contributor

piotrbak commented Sep 7, 2023

Acceptance Criteria:

  1. If image was excluded, we'll add the following attribute data-rocket-lazy-bg="excluded", that applied to source and attribute exclusions, not CSS external files.
  2. If image was processed correctly, it'll still have data-rocket-lazy-bg="loaded" attribute

@piotrbak piotrbak added type: enhancement Improvements that slightly enhance existing functionality and are fast to implement priority: medium Issues which are important, but no one will go out of business. needs: grooming module: lazyload background css images labels Sep 7, 2023
@piotrbak piotrbak added this to the 3.15.2 milestone Sep 21, 2023
@CrochetFeve0251
Copy link
Contributor

Reproduce

To reproduce there is just to exclude an image.

Root cause

The root cause from the issue is that for the moment excluded images are still in the mapping list.

Propose a solution

I will cut the solution into two parts.
The first one will be to remove the loaded attribute on the excluded element.
The second one will be to add the excluded attribute on the excluded element.

To prevent adding the loaded attribute on the excluded elements we will have to remove them from the mapping inside the TagGenerator class.

Then to add the excluded the strategy will be the following.
It would consist to add a tag with the mapping for excluded URLs inside the add_lazy_tag and add the logic to add the attribute excluded on each node which has an excluded attribute inside the JS script.

Estimate effort

Effort S

@CrochetFeve0251 CrochetFeve0251 added effort: [S] 1-2 days of estimated development time and removed needs: grooming labels Sep 25, 2023
github-merge-queue bot pushed a commit that referenced this issue Oct 9, 2023
Co-authored-by: Gael <gael@wp-media.me>
@engahmeds3ed engahmeds3ed mentioned this issue Oct 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
effort: [S] 1-2 days of estimated development time module: lazyload background css images priority: medium Issues which are important, but no one will go out of business. type: enhancement Improvements that slightly enhance existing functionality and are fast to implement
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants