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

Using with responsive images #4

Open
oyeanuj opened this issue Jul 16, 2017 · 7 comments
Open

Using with responsive images #4

oyeanuj opened this issue Jul 16, 2017 · 7 comments

Comments

@oyeanuj
Copy link
Contributor

oyeanuj commented Jul 16, 2017

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
Copy link
Contributor Author

oyeanuj commented Jul 17, 2017

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
Copy link
Owner

brycedorn commented Jul 17, 2017

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
Copy link
Contributor Author

oyeanuj commented Jul 18, 2017

@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
Copy link
Contributor Author

oyeanuj commented Jul 25, 2017

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

@brycedorn
Copy link
Owner

brycedorn commented Jul 25, 2017

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
Copy link
Contributor Author

oyeanuj commented Aug 16, 2017

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

@brycedorn
Copy link
Owner

brycedorn commented Sep 26, 2017

@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

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

No branches or pull requests

2 participants