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 slick-initialized on client-side only #560

Closed

Conversation

mkreidenweis
Copy link

@mkreidenweis mkreidenweis commented Nov 28, 2016

(This PR builds on the changes of #559; thus those changes are included in the diff as well.)

When using react-slick in isomorphic apps, the slider will be rendered before any client-side JavaScript has been run, leading to issues like #254, #508, or #537.

This PR provides a simple solution based on the initialized state that's already included in initial-state.js. It's a more React-like solution compared to #256.
By calling the setState in componentWillMount we prevent unnecessary re-rendering on pure client-side apps.

Compared to #288 this change implicitly takes the slidesToShow into account, as the slick-active class already handles that correctly.


After applying those changes, users will be able to optionally add the following styles in order to display the active slide while javascript has not been initialized yet:

.slick-slide.slick-active {
    display: block;
}

Edit: It's actually also necessary to add something like the following. Otherwise you end up with the active slide being shown twice.

.slick-track {
    width: 9000px;
}

@kimown
Copy link

kimown commented Dec 3, 2016

There is a problem about the style, when infinite: true, , server render will show two duplicated component.

From Here

#256 (comment)

@mkreidenweis
Copy link
Author

@kimown : This works fine for us, even in infinite mode. Server render will not render the slick-initialized class; thus the .slick-slide { display: none; } style from slick-carousel styles applies, hiding all the slides.
If you add the CSS mentioned above (.slick-slide.slick-active { display: block; }) only the active slides will actually be shown.
Only once the client JavaScript has been initialized, it adds the slick-initialized class, causing .slick-initialized .slick-slide { display: block; } to apply to all slides.

Could you give an example where it doesn't work for you?

@kimown
Copy link

kimown commented Dec 7, 2016

@mkreidenweis sorry too late to reply you, in my project, window is defined, so the condition fails, but I think it's better to use !!(typeof window === 'undefined' && window.document && window.document.createElement) to check the server render environment

@eliseumds
Copy link

@kimown window is defined? Well, maybe consider removing it, otherwise it may bring you a lot of trouble. Many projects do the simple typeof check.

@eliseumds
Copy link

No updates? :(

@brandon-arnold
Copy link

I have manually included these changes in my fork of react-slick, followed by running the build script and trying to link them in my app. Behavior is the same when the page loads: content for each carousel is stacked vertically before the paging mechanism kicks in and re-renders without the hidden pages, as you can see on brandonarnold.com.

@Jaikant
Copy link

Jaikant commented Jan 8, 2017

Hey, I am new to react-slick and am seeing this issue in my isomorphic app. The pictures stack up one below the other, before the carousel kicks in.
In the above discussion, I couldn't follow where to apply the css -> .slick-slide.slick-active { display: block; }.
An example would help.

@eliseumds
Copy link

@brandon-arnold @Jaikant guys, you'd have to manually include the mentioned CSS styles into your app's stylesheet.

@brandon-arnold
Copy link

@eliseumds I don't know what I'm missing; the .slick-slide.slick-active { display: block; } style does nothing for me.

simoneb added a commit to simoneb/react-slick that referenced this pull request Jan 9, 2017
simoneb added a commit to simoneb/react-slick that referenced this pull request Jan 9, 2017
@simoneb
Copy link

simoneb commented Jan 9, 2017

I couldn't wait on this to be accepted so I forked the project, included these changes (which BTW work great) and published on npm as react-slick-ssr (to avoid having to commit the lib folder into the repo).

@Jaikant
Copy link

Jaikant commented Jan 9, 2017

@simoneb thanks will try this out.

@mkreidenweis
Copy link
Author

@akiran : It would be great to get some feedback on this PR from you. Do you have any concerns that prevent you from merging this?

@simoneb
Copy link

simoneb commented Jan 18, 2017

@mkreidenweis even though I'm using this approach there's still the issue that if you try to display the initial slide immediately with

.slick-slide.slick-active {
    display: block;
}

then you'll get a duplicate copy as mentioned before by @kimown. Not sure if this happens only under certain circumstances though.

@mkreidenweis
Copy link
Author

@simoneb You're right. I was able to reproduce your issue now. There's actually another CSS style in our code that makes it work properly:

.slick-track {
    width: 9000px;
}

I didn't realize till now that this is essential for this server side rendering to work properly with the standard slick-carousel theme.

@Jaikant
Copy link

Jaikant commented Jan 18, 2017

@mkreidenweis @eliseumds the changes don't work for me. I am injecting the css on the server as well as included these changes

@simoneb
Copy link

simoneb commented Jan 25, 2017

@mkreidenweis as @Jaikant mentioned this fix doesn't work for me either, still get the same duplicate slide

@simoneb
Copy link

simoneb commented Jan 25, 2017

Ok so, what seems to work is this (ugly) fix:

.slick-slide.slick-active[data-index="0"] {
  display: block;
  width: 100%;
}

This gets rid of the duplicate slide by only selecting the first one and sets a correct width for it. This seems to work fine when applied on top of this PR and with this configuration:

{
  centerMode: true,
  centerPadding: '170px'
}

@mkreidenweis
Copy link
Author

@simoneb I happened to use the slider for content with fixed width only, so I guess that's the reason I didn't notice so far. Makes sense now.
@Jaikant Are you also experiencing your problems with slides that don't have a fixed width set? What does "doesn't work" mean exactly? Do you have samples/screenshots?

@brandon-arnold
Copy link

@mkreidenweis I also am using a flex-based width and am seeing the same results. You can observe behavior at brandonarnold.com.

@Jaikant
Copy link

Jaikant commented Jan 30, 2017

@mkreidenweis sorry for the delay in responding as I was not at work for the whole of last week. I worked around this by not rendering the component on the server as described here. I uploaded the behaviour I am seeing here.

@sscaff1
Copy link

sscaff1 commented Sep 23, 2017

@akiran any updates? It would be great if you could accept this PR.

@TLevesque
Copy link

I’m migrating my app to nextjs, does the SSR works now? Or should I use the fork https://www.npmjs.com/package/react-slick-ssr or something else? Any feedback?

Set `slideCount` and `currentSlide` during server-side rendering in
order to get the dots/pagers rendered.
So that CSS can distinguish between server-rendered state and
client-javascript-initialized state.
@sinxwal
Copy link

sinxwal commented Oct 27, 2017

typeof window === 'undefined' is not the guarantee that it is server rendering. Often people add fake window in ssr.

@mkreidenweis
Copy link
Author

So is there a better way to detect SSR than checking for presence of window?

@sinxwal
Copy link

sinxwal commented Oct 27, 2017

Smth like typeof window !== 'undefined' && window.document && window.document.createElement

@rajatjasuja23
Copy link

after adding the code ,the initial slider is coming fine with images ,css and dots ,but the slider functionality is not working and i cant able to move the slider in server side rendering.

@mkreidenweis
Copy link
Author

@rajatjasuja23 I'm not sure what you're trying to do. For the slider to become interactive it still needs React on the client side, even if the initial slide view is rendered on the server. So if you only render on the server, it won't give you an actual working slider.

@rajatjasuja23
Copy link

@mkreidenweis yes,my slider is working fine in client side but its taking 3-4 secs to load the actual working slider.My performance went down ,when i am doing ssr with react slick,i just want to improve the performance on server side,so is there any way to load the actual working slider in ssr ??,because there is a hyperlink on each slick slider item and in slow networks i cant able to redirect it to the next page in ssr it is only working when the actual working slider loads in client side

@laveesingh laveesingh closed this Jan 25, 2018
@brandon-arnold
Copy link

@laveesingh Was this fixed?

@laveesingh
Copy link
Contributor

@brandon-arnold all the ssr related issues are to be considered for release 0.19.

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.

None yet