-
Notifications
You must be signed in to change notification settings - Fork 32
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
Prevent slider from "flashing" when transitioning from SSR to CSR #14
Conversation
Hi! I'm VTEX IO CI/CD Bot and I'll be helping you to publish your app! 🤖 Please select which version do you want to release:
And then you just need to merge your PR when you are ready! There is no need to create a release commit/tag.
|
Beep boop 🤖 I noticed you didn't make any changes at the
In order to keep track, I'll create an issue if you decide now is not a good time
|
const handles = useCssHandles(CSS_HANDLES) | ||
const { list } = useListContext() | ||
|
||
const childrenArray = React.Children.toArray(children).concat(list) | ||
|
||
const isSlideVisibile = ( | ||
const isSlideVisible = ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🙏
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You shouldn't behave as a slider if you have less or equal items to a single page, it should only be simple list
react/components/Slider.tsx
Outdated
onTouchEnd={e => (usePagination ? onTouchEnd(e) : null)} | ||
onTouchMove={e => (usePagination ? onTouchMove(e) : null)} | ||
onTouchStart={e => | ||
usePagination && !shouldBeStaticList ? onTouchStart(e) : null |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be nice to change the name of onTouchStart
to handleTouchStart
. And this usePagination
variable is a bit confusing I read this and think that is a hook
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And you could encapsulate all of this code inside of the callback and always pass it
Your PR has been merged! App is being published. 🚀 After the publishing process has been completed (check #vtex-io-notifications) and doing A/B tests with the new version, you can deploy your release by running:
After that your app will be updated on all accounts. For more information on the deployment process check the docs. 📖 |
What does this PR do?
Refactors a few components and custom hooks from the
slider-layout
internal implementation that prevents it from "flashing" when transitioning from SSR to CSR.How to test it?
Both the Carousel and the Shelf in this workspace were replaced by list blocks using
slider-layout
s.Related to / Depends on
This is related to vtex-apps/product-summary#229 and vtex-apps/store-theme#190 .