Skip to content
New issue

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

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

Already on GitHub? # to your account

bug: use placeholder in carousel while loading next image #1085

Merged
merged 6 commits into from
Apr 1, 2019

Conversation

sirugh
Copy link
Contributor

@sirugh sirugh commented Mar 28, 2019

Description

The carousel just uses a simple <img> tag to load the images on the PDP. There is a moment that, depending on network speed, we jutter the page while loading a variant or new image. This PR handles that by adding an onLoad handler and some local state to the carousel class so that we render a placeholder while loading the image.

One thing I wasn't sure about was how the transparent placeholder we created in shared/images.js is styled or sized. It seems to render a few pixels larger than the actual image so there is still a slight jutter while we draw. That being said, this PR makes it much less jarring.

Related Issue

Closes #1084.

Verification Steps

  1. Go to a product page with variants.
  2. Click on a variant or size, or click next.
  3. The page should not jutter, and you should see a placeholder image in place of the carousel image while the next image loads.

How Have YOU Tested this?

I did the above.

Screenshots / Screen Captures (if appropriate):

My gif capture tool doesn't have a high enough fps to capture the jutter...:cry:

Proposed Labels for Change Type/Package

  • major (e.g x.0.0 - a breaking change)
  • minor (e.g 0.x.0 - a backwards compatible addition)
  • patch (e.g 0.0.x - a bug fix)

Checklist:

  • I have updated the documentation accordingly, if necessary.
  • I have added tests to cover my changes, if necessary.

@sirugh sirugh added bug Something isn't working pkg:venia-concept labels Mar 28, 2019
@vercel
Copy link

vercel bot commented Mar 28, 2019

This pull request is automatically deployed with Now.
To access deployments, click Details below or on the icon next to each push.

@tjwiebell tjwiebell self-assigned this Mar 29, 2019
@tjwiebell tjwiebell added the version: Patch This changeset includes backwards compatible bug fixes. label Mar 29, 2019
@vercel vercel bot temporarily deployed to staging March 29, 2019 15:16 Inactive
@coveralls
Copy link

coveralls commented Mar 29, 2019

Coverage Status

Coverage increased (+0.01%) to 76.528% when pulling 926c8bd on placeholderimages into b6e6a5c on develop.

Copy link
Contributor

@tjwiebell tjwiebell left a comment

Choose a reason for hiding this comment

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

Nice improvement. Discussed slight flash of placeholder with Stephen when switching between swatch colors, but fixing that would require a larger re-factor of the carousel component that doesn't seem necessary in this scope. Comfortable merging this as-is, and iterate on the slight flash in a future issue.

@dpatil-magento dpatil-magento merged commit 24ea2e5 into develop Apr 1, 2019
@supernova-at supernova-at deleted the placeholderimages branch July 25, 2019 15:18
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
bug Something isn't working pkg:venia-concept version: Patch This changeset includes backwards compatible bug fixes.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[bug]: When loading the main carousel image there is a FOUC while the image loads
5 participants