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

Add support for responsive images #264

Merged
merged 1 commit into from
Aug 8, 2021

Conversation

gaborbsd
Copy link
Contributor

@gaborbsd gaborbsd commented Jul 7, 2021

Nowadays it is important to support responsive images to save on bandwidth and create fast websites. It is really easy to support this with just allowing the sizes and srcset attributes for the img tag generated.

I found glightbox really handy and easy to use, just I felt that this was missing and went on to add it myself. It was also easy to read and understand the code. Thank you so much for sharing this nice lib with the world and thanks in advance for your consideration on accepting this PR.

@gingerchew
Copy link
Collaborator

We have been discussing this a bit in #248, but I do think the check for sizes is a good idea.

Maybe it's worth it to merge the two like:

// pseudo-code
if (hasSrcset && hasSizes) {
  doSrcsetAndSizes()
}

Since one is pointless without the other.

@gaborbsd
Copy link
Contributor Author

gaborbsd commented Jul 7, 2021

Thank you Frankie, I've modified the patch according to your suggestion. Btw, I forgot to mention that I also tested the change and it worked.

@tpeachman
Copy link

tpeachman commented Aug 5, 2021

I wouldn't necessarily merge srcset and sizes. I mean why would you require it? From what I know, there are use cases where you wouldnt need the sizes attribute.
Regardless, please add the support asap :D

@biati-digital biati-digital merged commit 4b6b4c9 into biati-digital:master Aug 8, 2021
@iamrobert
Copy link

iamrobert commented Aug 9, 2021

Thanks - the patch works well.

Here's my code implementation for a small, medium and large image sizes:

<a 
      href="images/large.jpg"  
      title="Responsive example" 
      data-title="Responsive example"
      data-description="Your browser will choose the optimal image for the resolution"
      class="glightbox"
      data-sizes="(max-width: 600px) 480px, (max-width: 1024px), (min-width: 1024px)"
      data-srcset="images/small.jpg 480w, images/medium.jpg 1023w, images/bb_files/large.jpg 1024w">
      GLIGHTBOX
</a>

@iamrobert
Copy link

@Frankie-tech Should this branch be in the /dist/ folder?

@gaborbsd
Copy link
Contributor Author

gaborbsd commented Aug 9, 2021

Thanks for merging this!

@gingerchew
Copy link
Collaborator

@qpido You are correct, you could also use it alone for just pixel density.

@iamrobert You are also right, the build script should have been run.

I'll make a new PR later today to fix both of those.

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 this pull request may close these issues.

5 participants