Skip to content

Fix slick-initialized to fire when the component finishes mounting …#256

Closed
chemoish wants to merge 1 commit intoakiran:masterfrom
chemoish:feature/fix-initialization
Closed

Fix slick-initialized to fire when the component finishes mounting …#256
chemoish wants to merge 1 commit intoakiran:masterfrom
chemoish:feature/fix-initialization

Conversation

@chemoish
Copy link
Copy Markdown

@chemoish chemoish commented Mar 4, 2016

…(Only fires on the client). This will prevent a FOUC when doing SSR.

NOTE: https://github.com/akiran/react-slick/blob/master/src/initial-state.js#L27 seems like dead code.

…(Only fires on the client). This will prevent a FOUC when doing SSR.
@chemoish
Copy link
Copy Markdown
Author

chemoish commented Mar 4, 2016

Addresses #254.

@chikathreesix
Copy link
Copy Markdown

Works great! Hiding the component is definitely better than FOUC. Ideally I'd like to prepare the space for the carousel and show a loading indicator in there.

@chemoish
Copy link
Copy Markdown
Author

@akiran thoughts?

@chemoish
Copy link
Copy Markdown
Author

@chikathreesix, for me slick-initialized is just wrong as it currently stands and should be fixed. #imo

@chikathreesix
Copy link
Copy Markdown

@chemoish I see your point. I have also tried a different approach after I commented here #288

@akiran
Copy link
Copy Markdown
Owner

akiran commented May 11, 2016

Closing this because add classes with classList.add is not a very react friendly
I might consider merging #288

@akiran akiran closed this May 11, 2016
@chemoish
Copy link
Copy Markdown
Author

Do you have documentation that indicates your statement is the case?

In my experience this is the most performant way of managing visibility. Alternatively, you can have a getInitialState and a setState that manages this information—it will cause a double render, but the virtual DOM should make the hit trivial.

Either solution is better than #288—due in part to the complexity of the solution, but also the current logic is incorrect (The slider isn't initialized until the component finishes mounting).

@chikathreesix
Copy link
Copy Markdown

chikathreesix commented May 12, 2016

The reason why I created #288 is, this solution hides the whole carousel so it still causes a layout change after the code runs. The logic of #288 is not complicated, it just hides non current banners. What makes it not really beautiful is handling duplicated banners placed on both left and right. In my opinion, the carousel should take another approach not to generate these backup banners but this is a different topic so I have just dealt with it in a not really beautiful way.

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.

3 participants