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.
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
[Announcement bar] Slides animation. #2595
[Announcement bar] Slides animation. #2595
Changes from all commits
91326a8
9b9de46
72ba2e2
beb668f
50069e5
8d729b6
1b40cdf
4443433
2575861
a683dec
aeae987
2931bc8
f472919
120d4f3
25600bc
7cf4a3e
932903b
80d3a90
b4b8fa2
cc6972d
7ef82a7
9327404
e4b07e7
dbdd4f4
6f767bd
94f243f
66b85cc
8dd224a
55f20ca
a06ec50
455d26e
6862e85
9975886
7412b11
1013873
1c4dd25
419b151
9af7783
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
One thing I noticed is that all the logic is added to be tackled onButtonClick when it should also be running on scroll/swipe 😬
If you use it on mobile and swipe instead of you will see that it doesn't apply all the changes necessary.
So I think you could either look into:
announcement-bar-slider--fade-***
class in liquid by default. Since you know which slide will be showing on page load and which ones won't.update()
function where it's checking what is the current page and what's the previous page. If you apply the logic you added toonButtonClick
there it should work in all cases. Tho it might be triggered too late compared to when the button is clicked 🤔From what I'm seeing it's working ok on page load. No animation is applied but at least all slides are being shown. If you click on the previous or next button though, and then swipe later, the slides are hidden. Ideally they would work whatever the method you're using to go to the next slide is.
RPReplay_Final1687551919.MP4
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.
Thanks for flagging this Ludo.
This is happening because when you start scrolling/swiping we don't apply animation but some slides still have animation classes because we remove them only with the next cycle. I added code to prevent it. Now it looks good to me.
I would not apply animation for cases when we scroll/swipe because I think it looks better and more natural to have the effect we have now for scrolling.
Thoughts?
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 having the animation on scroll/swipe would be good but I don't think we should delay this PR more for it. It could be a thing we look into adding in the future.
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.
shouldn't we use the new function you added here ?
setSlidePosition(position)
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.
inside itself?
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.
Oh no, I didn't realize. It got me confused cause the function is so similar for the SliderComponent and SlideshowComponent. And named the same way as well though does it differently, one with a timeout and one without.