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

Handle broken image uri #137

Closed
serfgy opened this issue Sep 11, 2018 · 11 comments · Fixed by #138
Closed

Handle broken image uri #137

serfgy opened this issue Sep 11, 2018 · 11 comments · Fixed by #138
Assignees

Comments

@serfgy
Copy link

serfgy commented Sep 11, 2018

Is there a way to handle broken image uri? In the event the src value leads to an unavailable image, the broken image pic appears and can be unsightly.

Also, clicking on the broken image pic still zooms in on it.

@rpearce
Copy link
Owner

rpearce commented Sep 11, 2018

@serfgy thanks for asking your question! There currently is not a way, but perhaps there should be. What do you envision? Accepting callbacks for the Image onerror? Having a background color?

@serfgy
Copy link
Author

serfgy commented Sep 12, 2018

If the image link is broken or picture is not found, how <Img> (npm react-image) handles it is that <img> will not be rendered, I'm not quite sure how to do this with <ImageZoom> but I can share my current workaround:

                  <ImageZoom
                    image={{
                      src: window.formAppConfig.imageEntry + '/' + text + '.jpg',
                      style: { margin: "-12px", width: "36px", height: "36px" },
                      onError: (e) => { e.target.src = 'http://xxx.com/1x1.png' } 
                    }}
                  />

onError works because it's an existing prop of <img>, this however does not prevent <ImageZoom> from rendering but instead replaces the broken image (post compile I believe), so if we click on the blank 1x1.png pixel image it still zooms out to a blank (white) page, which can be confusing. The best case scenario would be that <ImageZoom> self checks like <Img> and would not render in the first place. Maybe we can add into the options for a choice to not render if the image link is broken, or a Boolean prop on <ImageZoom> like renderBrokenImg=false.

https://github.com/mbrevda/react-image/blob/master/src/index.js

By the way, awesome work!

@rpearce
Copy link
Owner

rpearce commented Sep 12, 2018

Instinct tells me that if loading the initial image fails, then zooming should be disabled by default. Agree/disagree?

Edit: and thanks for the props!

@serfgy
Copy link
Author

serfgy commented Sep 12, 2018

Yea, the priority is to disable the zoom by default for the broken link case, even if we cant stop the rendering for now. Cheers!

@rpearce
Copy link
Owner

rpearce commented Sep 12, 2018

K I'll schedule some time for this and get back to it soon. Thanks for your submission, and I'll add you to the All Contributors list in the README

@rpearce
Copy link
Owner

rpearce commented Sep 15, 2018

Looking to get on this tomorrow at some point (NZ-time)

@rpearce
Copy link
Owner

rpearce commented Sep 16, 2018

Update: almost done and should have a PR up either tonight or tomorrow

@rpearce
Copy link
Owner

rpearce commented Sep 16, 2018

@serfgy can you please check out #138? It should allow for:

  • onLoad and onError callbacks passed with the image prop should work
  • onError, all zoom capabilities and whatnot should be disabled
  • if the image's src changes, then it'll try to fetch that (I added an example of this in the React Storybook)

@rpearce rpearce self-assigned this Sep 16, 2018
@serfgy
Copy link
Author

serfgy commented Sep 17, 2018

@rpearce , just tested it, it detects failed images and disables the zoom nicely!

I tried removing the timeout and put the src change on the onError instead (on the second image in the storybook) and it also works with no issues:

          onError: a => {
            a.target.src = 'https://rpearce.github.io/react-medium-image-zoom/bridge.jpg';
          }

Cheers!

@rpearce
Copy link
Owner

rpearce commented Sep 17, 2018

Cool. I'll merge it up and notify you when it's published

@rpearce
Copy link
Owner

rpearce commented Sep 17, 2018

@serfgy I've just published v3.0.14 and added you to the "all contributors" list: https://github.com/rpearce/react-medium-image-zoom#contributors.

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

Successfully merging a pull request may close this issue.

2 participants