search

briangonzalez / jquery.adaptive-backgrounds.js

🦎 A jQuery plugin for extracting the dominant color from images and applying the color to their parent.
http://briangonzalez.github.io/jquery.adaptive-backgrounds.js/
6.54k stars 454 forks source link

Add support for Picture element #67

Closed IgorUsoltsev closed 7 years ago

IgorUsoltsev commented 8 years ago

Added support for Picture element for this brilliant plugin.

NOTE: your element is required to have at least 1 <img> (it is the only valid markup for this element but still)

briangonzalez commented 8 years ago

@IgorUsoltsev Mind adding in a demo?

https://github.com/briangonzalez/jquery.adaptive-backgrounds.js/tree/master/demos

briangonzalez commented 8 years ago

Oh, and thanks for the PR!

IgorUsoltsev commented 8 years ago

Hey, sure, feel free to use

Glad it will help someone

briangonzalez commented 8 years ago

Would you be willing to create a demo @IgorUsoltsev ?

IgorUsoltsev commented 8 years ago

Ah sorry, didn't get you. Np will make in few days

On Sep 12, 2016 20:10, "Brian Gonzalez" notifications@github.com wrote:

Would you be willing to create a demo @IgorUsoltsev https://github.com/IgorUsoltsev ?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/briangonzalez/jquery.adaptive-backgrounds.js/pull/67#issuecomment-246416063, or mute the thread https://github.com/notifications/unsubscribe-auth/AFpKPaNtDk1O4PXC1wCtSAnq5xwuo4xfks5qpYXdgaJpZM4J6TXy .

IgorUsoltsev commented 8 years ago

Hey @briangonzalez

Just pushed the demo

AlfredJKwack commented 7 years ago

Hi,

Testing this out I note there are some browser support issues on Chrome (v58 on MacOS).

Since the current demo didn't entirely mimic the spec, I made some changes to it. According to the spec, multiple source elements (with appropriate media attributes) are allowed for a single image element.

Assume the below change to the demo and a window size larger than 600px. 

  <div class='img-wrap'>
      <picture id="img" data-adaptive-background>
          <source srcset="images/2.jpg" media="(max-width: 599px)">
          <source srcset="images/3.jpg" media="(min-width: 600px)">
          <img src="images/1.jpg">
      </picture>
  </div>

The resulting image looks like the below. As you can see, it takes the background color from 1.jpg while what's being displayed is 3.jpg. This is not what one would expect.

When I reload the page several times with one or multiple images I get different results which indicates inconsistency in Chrome. Testing this out in Safari however posed no such problems and it ran as designed.

@briangonzalez Appreciate your thoughts. As far as I can tell the code is not at issue here.

scap

AlfredJKwack commented 7 years ago

Digging a little further I found that the Chrome team decided to:

Update currentSrc just before HTMLImageElement.onload During the spec discussions [1], we have come to the conclusion that currentSrc should be updated only before onload/onerror are triggered. This CL implements that behavior.

I've also made a quick CodePen to illustrate behaviour differences between browsers. Pulling up the console in Chrome I see that sometimes the second log call comes up with an empty string, sometimes not...

My conclusion here would be that the code is probably fine but we be clear that an appropriate tigger point (ie. the difference between window.onload vs $(document).ready() ) will be key to how things can be expected to behave.

scrap

briangonzalez commented 7 years ago

@AlfredJKwack This diff is quite small and comes with a demo. I am cool shipping it.

Thanks for all of your hard work.