search

brycedorn / react-intense

A React component for viewing large images up close 🔍
https://bryce.io/react-intense
MIT License
187 stars 10 forks source link

Using with responsive images #4

Open oyeanuj opened 7 years ago

oyeanuj commented 7 years ago

Hey @brycedorn, thanks for the React port!

I just opened an issue in the main library before I remembered your port. I'm using responsive images in my app so far (using srcset & picture) and wondering how does react-intense play with that paradigm?

Thank you!

oyeanuj commented 7 years ago

To clarify, it would entail adding srcset and sizes attribute to the img tag so that appropriate images are loaded on mobile vs web.

brycedorn commented 7 years ago

hey @oyeanuj, good question! I haven't added any features around different image sizes (just thumbnail and full-size), but since the main image is lazily loaded it'd be a pretty simple addition to renderViewer() to compare width against various srcset params and select the proper src.

feel free to open a PR, otherwise I'll get around to this as I have time!

oyeanuj commented 7 years ago

@brycedorn On a little further investigation with the goal of a PR, I had two questions -

  1. The thumbnail image is set as a background on the <a> tag rather than an <img>. This makes it impossible to specify srcSet instead of a single thumbnail source. Wondering what the thought process was behind the decision to use <a> background instead of an <img>?

  2. The more I look, the more it seems like the viewer should be its own component that can be imported on its own with or without the thumbnail component. Apart from making the code more modular, this would mean that the user could have the link on any component/button in their app, and on clicking it, the viewer component is displayed (whether in a modal or not). Is that a direction you'd pick?

Thoughts?

oyeanuj commented 7 years ago

@brycedorn Just checking in if you had any thoughts on the above? Thank you!

brycedorn commented 7 years ago

hey @oyeanuj, good ideas!

  1. no real reason, it's just more concise to have one element, & given that there's no srcSet prop for the component there wasn't a need for it.

  2. I agree, the thumbnail triggers should be abstracted to a separate component with onClick & src passed in as props!

oyeanuj commented 7 years ago

@brycedorn Just curious if you see this on your radar anytime soon?

brycedorn commented 7 years ago

@oyeanuj pulled the thumbnail (trigger) component into an optional prop - didn't implement srcSet as that's something that can just be controlled outside of the component itself, to keep things simpler