Skip to content
This repository has been archived by the owner on Mar 12, 2020. It is now read-only.

[MX-166] handle images without deep zoom #2066

Merged
merged 3 commits into from
Jan 30, 2020

Conversation

ds300
Copy link
Contributor

@ds300 ds300 commented Jan 30, 2020

https://artsyproduct.atlassian.net/browse/MX-166

To recap: the problem was that some images in our system don't have a deepZoom property for mysterious reasons. This was causing an uncaught error in the full screen carousel component which crashed the artwork page, showing the ol' White Screen Of Death 'retry' screen.

Working around this was complicated because what if an artwork with multiple images had just one of the images lacking deepZoom data? The full screen carousel lets you swipe between deepZoom images and is not set up to handle the deepZoom info not being present. So as a quick fix for this (seemingly very very rare) situation was as follows:

  • We ignore any artwork images that don't have deepZoom.
  • If that leaves no images remaining we show the first one.
  • If we are showing just one image which doesn't have deepZoom, then when a user taps on the image we don't go full screen.

It ain't perfect (leads to us potentially not showing images a partner has uploaded) but it'll at least prevent WSOD situations.

I think the other quick-and-dirty fix would be to disable deep zoom if an artwork has at least one image lacking deepZoom info, but given how rare this is I'm not too bothered either way.

Copy link
Contributor

@ashfurrow ashfurrow left a comment

Choose a reason for hiding this comment

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

Nice stuff, thanks!


if (result.some(image => !image.deepZoom)) {
if (!__DEV__) {
Sentry.captureMessage(`No deep zoom for at least one image on artwork (see breadcrumbs for artwork slug)`)
Copy link
Contributor

Choose a reason for hiding this comment

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

🙌

@ashfurrow
Copy link
Contributor

Following up on the deeper (get it?) discussion, I think the approach you took makes sense. Given how rare this is, the slightly degraded UX is acceptable to me (and certainly much better than WSOD). Thanks for outlining the trade-offs 👍 #mergeongreen

@artsy-peril artsy-peril bot merged commit 0dca856 into master Jan 30, 2020
@artsyit
Copy link
Collaborator

artsyit commented Jan 30, 2020

🚀 PR was released in v1.21.30 🚀

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants