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

Use slots for the images instead of attribute. #6

Open
enjikaka opened this issue May 27, 2019 · 3 comments
Open

Use slots for the images instead of attribute. #6

enjikaka opened this issue May 27, 2019 · 3 comments

Comments

@enjikaka
Copy link

Having the images specced in an attribute is a bit too React-ish. Consider using slots instead. This would also give to possibility to use and for assigning srcset for responsive images or using WebP with fallback etc :)

<ken-burns-carousel>
   <slot>
      <img src="https://source.unsplash.com/Qh9Swf_8DyA" alt="">
      <img src="https://source.unsplash.com/O453M2Liufs" alt="">
   </slot>
</ken-burns-carousel>

For img elements getting the imageURL could be done like so: https://codepen.io/enjikaka/pen/mYKWGP?editors=0010

@leolabs
Copy link
Member

leolabs commented Jun 6, 2019

Hey @enjikaka,
that's a good idea! If you'd like to make a PR for this, please feel free to do so. We don't have a lot of time on our hands at the moment, so it could take a while for us to make these changes ourselves.

Your code example looks good. I'd maybe only wait for the first image to load before showing the carousel as that could minimize the initial load time.

If you have any questions, contact me anytime :)

@NeoLegends
Copy link
Member

We‘d also need to see how we can wrap each image in a div to make the animations fast. Not sure if this works properly with slots.

@leolabs
Copy link
Member

leolabs commented Jun 6, 2019

That should be possible. We could just wrap every image element in the slot with our custom div before inserting them into the carousel. It would be interesting though to see how changes in the slot can be smoothly applied as we do it at the moment.

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

No branches or pull requests

3 participants