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

Lazy Images: Do not change image display for no JS support #9872

Merged
merged 2 commits into from
Jul 9, 2018

Conversation

ebinnion
Copy link
Contributor

@ebinnion ebinnion commented Jul 5, 2018

To support browsers without JavaScript enabled, we made a change to hide lazily loaded images by default and then add a class with JavaScript which would show the lazy loaded image. This was done in #9780.

When released, at least two users reported issues with their images no longer being centered. This is because we were setting the display to inline-block when we decided to show the image. This PR changes that behavior so that we set display to block instead.

Ideally, we would hide the lazily loaded images by appending a class to the HTML element when the post is rendered. Then, we could just remove the class with JavaScript. That being said, I am not aware of a great way to do this in WordPress. I just pushed a second commit to do just that. Essentially, I use a not selector on the html element to hide the image, and then use JavaScript to set the class on the html element which will show the lazy loaded image.

To test:

  • Checkout branch
  • Ensure lazy images module is on
  • Load a post with images in it, with at least one image being centered
  • Ensure the centered image does not fill the post content. If it does, crop it
  • Reload post and ensure that image is properly centered

Before:

before 9872

After:

after 9872

Fixes: #9879

@ebinnion ebinnion added [Type] Bug When a feature is broken and / or not performing as intended [Status] Needs Review To request a review from Crew. Label will be renamed soon. [Feature] Lazy Images labels Jul 5, 2018
@ebinnion ebinnion self-assigned this Jul 5, 2018
@ebinnion ebinnion requested review from zinigor and oskosk July 5, 2018 19:07
@ebinnion ebinnion requested a review from a team as a code owner July 5, 2018 19:07
@jetpackbot
Copy link

jetpackbot commented Jul 5, 2018

Warnings
⚠️

"Testing instructions" are missing for this PR. Please add some

Generated by 🚫 dangerJS

@oskosk oskosk added this to the 6.4 milestone Jul 5, 2018
@ebinnion ebinnion force-pushed the update/lazy-images-no-js-css-fix branch from 246952c to b9f95c6 Compare July 5, 2018 19:43
@ebinnion ebinnion changed the title Lazy Images: Set display to block when showing lazy images Lazy Images Jul 5, 2018
@ebinnion ebinnion changed the title Lazy Images Lazy Images: Do not change image display for no JS support Jul 5, 2018
Copy link
Contributor

@brbrr brbrr left a comment

Choose a reason for hiding this comment

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

Tested on both desktop & mobile screensizes

LGTM 🚢

@brbrr brbrr added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review To request a review from Crew. Label will be renamed soon. labels Jul 9, 2018
@ebinnion ebinnion merged commit aea1fee into master Jul 9, 2018
@ebinnion ebinnion deleted the update/lazy-images-no-js-css-fix branch July 9, 2018 18:06
@ghost ghost removed the [Status] Ready to Merge Go ahead, you can push that green button! label Jul 9, 2018
oskosk pushed a commit that referenced this pull request Jul 31, 2018
oskosk added a commit that referenced this pull request Jul 31, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Lazy Images [Type] Bug When a feature is broken and / or not performing as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants