Fix SSR of cloned slides #444
Merged
Conversation
This file contains 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
Tofandel
force-pushed
the
fix/ssr-clones
branch
2 times, most recently
from
57338fb to
e0d0a77
Compare
Tofandel
force-pushed
the
fix/ssr-clones
branch
7 times, most recently
from
0d07990 to
3a0ba32
Compare
Tofandel
force-pushed
the
fix/ssr-clones
branch
from
3a0ba32 to
569dd7f
Compare
|
Unfortunately the issue still exists
|
|
I can see an edge case where if we have itemsToShow > nb of slides with wraparound, we will not get enough slides in the clones I'm seeing if we can use a for instead of looping on slides and using a placeholder fragments if we can't find a slide to solve both of this |
|
Found an elegant solution and also checked the docs, it all checks out |
Tofandel
force-pushed
the
fix/ssr-clones
branch
from
09c5d53 to
64db019
Compare
Tofandel
commented
Dec 8, 2024
| </div>" | ||
| `; | ||
|
|
||
| exports[`SSR Carousel > renders server side properly 2`] = `"<div id="app"><section class="carousel is-ltr" style="" dir="ltr" aria-label="Gallery" tabindex="0"><div class="carousel__viewport"><ol class="carousel__track" style="transform:translateX(0px);"><li style="width:50%;" class="carousel__slide carousel__slide--clone" aria-hidden="true"></li><li style="width:50%;" class="carousel__slide carousel__slide--clone" aria-hidden="true"></li><li style="width:50%;" class="carousel__slide carousel__slide--clone" aria-hidden="true"></li><!--[--><li style="width:50%;" class="carousel__slide carousel__slide--visible carousel__slide--prev" id="v-0">1 <input type="text"></li><li style="width:50%;" class="carousel__slide carousel__slide--visible carousel__slide--active" id="v-1">2 <input type="text"></li><li style="width:50%;" class="carousel__slide carousel__slide--visible carousel__slide--next" id="v-2">3 <input type="text"></li><li style="width:50%;" class="carousel__slide" id="v-3">4 <input type="text"></li><li style="width:50%;" class="carousel__slide" id="v-4">5 <input type="text"></li><!--]--><li style="width:50%;" class="carousel__slide carousel__slide--clone carousel__slide--visible carousel__slide--prev" aria-hidden="true"></li><li style="width:50%;" class="carousel__slide carousel__slide--clone carousel__slide--visible carousel__slide--active" aria-hidden="true"></li><li style="width:50%;" class="carousel__slide carousel__slide--clone carousel__slide--visible carousel__slide--next" aria-hidden="true"></li></ol></div><!--[--><!--[--><button type="button" class="carousel__prev" aria-label="Navigate to previous slide" title="Navigate to previous slide"><svg class="carousel__icon" viewBox="0 0 24 24" role="img" aria-label="Arrow pointing to the left"><title>Arrow pointing to the left</title><path d="M15.41 16.59L10.83 12l4.58-4.59L14 6l-6 6 6 6 1.41-1.41z"></path></svg></button><button type="button" class="carousel__next" aria-label="Navigate to next slide" title="Navigate to next slide"><svg class="carousel__icon" viewBox="0 0 24 24" role="img" aria-label="Arrow pointing to the right"><title>Arrow pointing to the right</title><path d="M8.59 16.59L13.17 12 8.59 7.41 10 6l6 6-6 6-1.41-1.41z"></path></svg></button><!--]--><ol class="carousel__pagination"><li class="carousel__pagination-item"><button type="button" class="carousel__pagination-button" aria-label="Navigate to slide 1" aria-pressed="false" aria-controls="v-0" title="Navigate to slide 1"></button></li><li class="carousel__pagination-item"><button type="button" class="carousel__pagination-button carousel__pagination-button--active" aria-label="Navigate to slide 2" aria-pressed="true" aria-controls="v-1" title="Navigate to slide 2"></button></li><li class="carousel__pagination-item"><button type="button" class="carousel__pagination-button" aria-label="Navigate to slide 3" aria-pressed="false" aria-controls="v-2" title="Navigate to slide 3"></button></li><li class="carousel__pagination-item"><button type="button" class="carousel__pagination-button" aria-label="Navigate to slide 4" aria-pressed="false" aria-controls="v-3" title="Navigate to slide 4"></button></li><li class="carousel__pagination-item"><button type="button" class="carousel__pagination-button" aria-label="Navigate to slide 5" aria-pressed="false" aria-controls="v-4" title="Navigate to slide 5"></button></li></ol><!--]--><div class="carousel__liveregion carousel__sr-only" aria-live="polite" aria-atomic="true">Item 2 of 5</div></section></div>"`; |
There was a problem hiding this comment.
The SSR now doesn't contain the content of the cloned slide but this doesn't trigger a mismatch because the node is properly hydrated with it's key and so it can then be properly updated when slides.length > 0
It would be possible to add the getSlidesVNodes now to have the content in SSR for slides not within components as well as it won't trigger a mismatch but that means a different behavior if slotted or not, and that might lead to confusion
There was a problem hiding this comment.
In #443, I implemented the generation of only the cloned slides from getSlidesVNodes, leaving the component-based ones unchanged. However, your approach of falling back to an empty slide offers a more consistent and predictable behavior. I appreciate the simplicity and clarity it brings to the implementation. I’ll go ahead with releasing this update and gather feedback from the community to ensure it aligns with their expectations.
Tofandel
force-pushed
the
fix/ssr-clones
branch
6 times, most recently
from
bf897b6 to
f6fff36
Compare
Tofandel
force-pushed
the
fix/ssr-clones
branch
from
f6fff36 to
987a307
Compare
This was referenced Dec 9, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Superseedes #443
Fixes #442
I added a test for SSR and found an issue with the index calculation of pre cloned slides