You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
This PR adds a new static config prop, which when true will skip rendering of a React portal, and simply render children as they are passed into the carousel. This smooths the handshake between client / server rendering, however with the tradeoff: when static, the carousel cannot be modified at runtime (e.g., slides cannot be dynamically added or removed). For many usecases this tradeoff is acceptable. See this comment for more info.
Description of problem
While attempting to implement the carousel component, we noticed that there was a flicker when attaching to the DOM from an SSR pass:
Initially we thought that it was an issue with the underlying flickity instance on init, but digging deeper found that the problem came down to this bit of code, in react-flickity-component:
renderPortal(){if(!this.carousel)returnnull;
...
During the initialization phase, the carousel instance hasn't yet been instantiated, and so null is returned from within react, leading to empty content on the page. When things init, the code proceeds and things execute correctly, albeit with a flicker.
However, when playing more with the implementation we found that the portal is unnecessary, even if the carousel is only rendered client-side. By simply returning this.props.children in the render method
Thanks for your contribution. Unfortunately we do have to use portal to render children. Your code works perfectly fine if your slider items are static, meaning no adding or deleting items. But if they are dynamic, it would break the component because after Flickity is initialized, the children are not rendered in the original DOM hierarchy and we have to use Portal to render them correctly.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR adds a new
staticconfig prop, which when true will skip rendering of a React portal, and simply render children as they are passed into the carousel. This smooths the handshake between client / server rendering, however with the tradeoff: whenstatic, the carousel cannot be modified at runtime (e.g., slides cannot be dynamically added or removed). For many usecases this tradeoff is acceptable. See this comment for more info.Description of problem
While attempting to implement the carousel component, we noticed that there was a flicker when attaching to the DOM from an SSR pass:
Initially we thought that it was an issue with the underlying flickity instance on init, but digging deeper found that the problem came down to this bit of code, in
react-flickity-component:During the initialization phase, the carousel instance hasn't yet been instantiated, and so
nullis returned from within react, leading to empty content on the page. When things init, the code proceeds and things execute correctly, albeit with a flicker.However, when playing more with the implementation we found that the portal is unnecessary, even if the carousel is only rendered client-side. By simply returning
this.props.childrenin therendermethodeverything attaches and initializes as it should, with the added benefit that it also works correctly during the server -> client handshake pass.
@theolampert - Wondering your thoughts? Why was
renderPortalused to begin with? By removing it everything seems to work a-ok:No flicker:
We've only been able to test this for our use-case but yeah! Any feedback would be appreciated.
(If you're curious about our SSR implementation, see here.)