-
Notifications
You must be signed in to change notification settings - Fork 150
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
fix: [] Fix Shopify not displaying missing SKUs for collections/products #506
fix: [] Fix Shopify not displaying missing SKUs for collections/products #506
Conversation
const [imageHasLoaded, setImageLoaded] = useState(false); | ||
const [imageHasErrored, setImageHasErrored] = useState(false); | ||
const [imageHasErrored, setImageHasErrored] = useState(!product.image); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure I understand the reason for this change
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey John, without that code, when we pass a product that has no image (happens when for example the sku can no longer be found in the shop), the component unnecessarily renders an img
with an empty src, only to then immediately error and set this boolean to true. By immediately setting it to true, we prevent that unnecessary dance.
Just a small optimization, but not important if you think it makes the code harder to read.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I might instead leave this as is, make the following change below - because it makes a bit more sense to me, and avoids imageHasErrored
actually meaning two different things. I leave it up to you though :)
{!product.image || imageHasErrored ? (
<div className={styles.errorImage}>
<Icon icon={productIsMissing ? 'ErrorCircle' : 'Asset'} />
</div>
) : (
<div className={styles.imageWrapper(imageHasLoaded)}>
<img
style={{ display: imageHasLoaded ? 'block' : 'none' }}
onLoad={() => setImageLoaded(true)}
onError={() => setImageHasErrored(true)}
src={product.image}
alt={product.name}
data-test-id="image"
/>
</div>
)}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A great suggestion, implemented :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changes look good!
Hello, this PR solves three issues:
1.) For collections preview we were fetching first 20 collections meaning that for stores with more than 20 collections the preview would break. I have increased that limit to 250 (max) as Shopify unfortunately does not support fetching multiple collections by ids
2.) This PR also makes sure that missing collections/products (if they are deleted for example) still show up in the interface. This was always the case for product variants, but I missed adding it for products/collections.
3.) I have also amended the
ecommerce-app-base
to display the correct sku type in the "missing" tag.Thanks :)