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] image isLoaded state was being set to false after it was set to true … #1618

Merged
merged 4 commits into from
Sep 4, 2019

Conversation

sirugh
Copy link
Contributor

@sirugh sirugh commented Aug 29, 2019

Image isLoaded state was incorrectly being set to false. This PR removes the effect that was, for some reason, doing that. We don't need to set state to false on mount because that's the default state of the useState.

The reason this occurs on develop locally is that the images are loaded fast enough that the mount effect was happening after the load, which means we set isLoaded to true and then we trigger the effect which sets isLoaded to false.

This is definitely a bug and will be seen with a fast image server.

BEFORE

Image from Gyazo

AFTER

Image from Gyazo

@vercel
Copy link

vercel bot commented Aug 29, 2019

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

Latest deployment for this branch: https://pwa-studio-git-sirugh-fix-image-placeholder-state.magento.now.sh

@PWAStudioBot
Copy link
Contributor

PWAStudioBot commented Aug 29, 2019

Fails
🚫 Missing "Description" section. Please add it back, with detail.
🚫 Missing "Verification Steps" section. Please add it back, with detail.
🚫

No linked issue found. Please link a relevant open issue by adding the text "closes #<issue_number>" in your PR.

Messages
📖

Access a deployed version of this PR here. Make sure to wait for the "pwa-pull-request-deploy" job to complete.

📖 DangerCI Failures related to missing labels/description/linked issues/etc will persist until the next push or next nightly build run (assuming they are fixed).

If your PR is missing information, check against the original template here. At a minimum you must have the section headers from the template and provide some information in each section.

Generated by 🚫 dangerJS against 91dda65

@sirugh sirugh added the version: Patch This changeset includes backwards compatible bug fixes. label Aug 29, 2019
// On mount, reset loaded to false.
useEffect(() => {
setIsLoaded(false);
}, [src]);
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not just on the mount but for all cases when src changes. Makes sense because when src changes, image need to be re-fetched. But since we are not handling the fetch part, it is safe to remove this block of code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right! Thank you for that.

@revanth0212
Copy link
Contributor

This might be the reason why images fail to load on smaller devices in case #1584.

@sirugh sirugh changed the title image isLoaded state was being set to false after it was set to true … [bug] image isLoaded state was being set to false after it was set to true … Aug 30, 2019
@sirugh sirugh added bug Something isn't working Progress: approved and removed Progress: review labels Aug 30, 2019
@vercel vercel bot requested a deployment to staging September 3, 2019 16:41 Abandoned
@dpatil-magento dpatil-magento merged commit 818400a into develop Sep 4, 2019
@dpatil-magento dpatil-magento deleted the sirugh/fix-image-placeholder-state branch September 4, 2019 17:12
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
bug Something isn't working pkg:venia-ui version: Patch This changeset includes backwards compatible bug fixes.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants