search

akiran / react-slick

React carousel component
http://react-slick.neostack.com/
MIT License
11.75k stars 2.11k forks source link

Lazy load an offset amount of images #390

Closed gkilian closed 6 years ago

gkilian commented 8 years ago

Proposal:

Allow for an offset of images to be lazy loaded, rather than just the one.

Problem:

Currently when swiping through a set of images with lazyLoad set to true, the next image in the queue doesn't start loading until you've committed to the swipe. This causes a second (like in the react-slick examples) or more (when using large images) of no image loaded in the slide.

Solution:

By implementing something along the lines of lazyLoadOffset, we can add a ranges of images to be loaded from the start. Then progressively load the others as the user continues to swipe.

I'm not sure if the community is interested in this functionality. My team has implemented this in our fork of react-slick and are willing to help port over the code if wanted.

akiran commented 8 years ago

This issue is fixed in latest release

gkilian commented 8 years ago

@akiran - Can we re-open this? I don't believe that this is resolved.

Although it does seem to quickly load the next image, it's still not loading the image in advance. Thus, while you are in swiping the next images is not there.

This works fine in the Demo example because the next image is off screen, but it doesn't work so well when images are back to back.

janicklas-ralph commented 8 years ago

Any update on this? Would be really nice to have this.

i8ramin commented 7 years ago

Hello. Wondering if there was any progress made on this issue

emilbruckner commented 7 years ago

This is essential

markshust commented 7 years ago

@gkilian do you have the code for this? appears this wasn't pulled into this repo yet. i could really use this functionality. cheers -m

gkilian commented 7 years ago

@markoshust - I do in our Condenast fork. I'll submit a PR and tag the people in the PR for review

SBRK commented 7 years ago

Would love this as well

gkilian commented 6 years ago

@akiran - Our implementation on Conde requires the use of a added prop lazyLoadOffset. A secondary PR of mine was closed due to the fact that it supported a non slick-js prop. https://github.com/akiran/react-slick/issues/429

Would this be the case for this issue as well? I would hate to write a PR if the end result is the same.

laveesingh commented 6 years ago

I'm afraid we cannot accept props that deviate from slick libary as for now.

gkilian commented 6 years ago

@akiran - can we have a discussion on how we can work together to rectify these issues, instead of denying them and closing. Our end goal at Conde is to move back to this library over mantaining our fork. In doing so, we need to rectify these issues.

From the conversations on this issue and in https://github.com/akiran/react-slick/issues/429 - it seems that this plagues more than just our organization.

Alternative solution

If we're going in a direction where we're not introducing props which aren't native to slick-js are we comfortable with defaulting the offset to 1? I understand this is the current functionality, however it's not working properly.

akiran commented 6 years ago

@gkilian We are strict about not adding new props because that will increase maintenance burden over long terms. We are already running into issues where fix to one mode is effecting other modes. Adding more options will only make it worse. We are trying our best to fix existing issues.

We will be happy to accept PRs that fixes the issues with out adding new props at the moment.

For this case, will it be possible to implement prefetching logic in the wrapper component of Slider. We will be happen add such solutions to documentation

vaibhavi3t commented 6 years ago

@akiran I'm also using the react-slick and I want to load the offset amount of images. Is there any possibility of coming this feature in near future?

sergiubologa commented 5 years ago

Hello. Any updates on this issue? Can we at least set the default offset to 3 instead of 1?

joshdavenport commented 5 years ago

This would be such a great addition, any hints as to where to start on this if there isn't any existing work already ongoing?

gkilian commented 5 years ago

@sergiubologa @joshdavenport - It seems the approach we took at Conde, of adding a prop, is a no go in this repo. Let me see what I can do to get a public version of our fork on my account and highlight the changes we made to get this done.

Possible alt. solution: I could submit a PR without adding the prop, but I'd need the accepted offset amount. If that's 1 then I could also just add that.

sergiubologa commented 5 years ago

There's a new PR trying to solve this issue: https://github.com/akiran/react-slick/pull/1496

The problem is the last approved PR was 2 months ago 😞

joshdavenport commented 5 years ago

@gkilian either of those actions would be worth doing imo.

In the context of this repo, surely even just lazyloading ahead by one by default when using ondemand is totally acceptable without giving the ability to change. Struggle to see that anybody would see that as a bad thing.

Definitely appears there is a hunger here for a more opinionated version of this slick component where there's scope to add nice features on top. Totally understand the difficulty though, as the comment from @akiran indicates. Perhaps it is more effort than it's worth. 🤷‍♂️

mledwards commented 1 year ago

I found a pretty nice work around to get the previous and next images lazy loading. Add centerMode: true, centerPadding: 1px, so the left and right images are showing one pixel, then set margin: 0 -1px and overflow: hidden on the parent so it hides the images again.

Looks identical to the default slider but with 1 offset image lazy loading.