Skip to content
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

[Slideshow] prevent vertical overflow on slideshow #3455

Merged
merged 1 commit into from
May 9, 2024

Conversation

ludoboludo
Copy link
Contributor

PR Summary:

In some instances of the slideshow, there can be some odd behaviours happening with some scrolling on the media when it shouldn't.
This is to prevent this behaviour.

To replicate the issue you would need to use an image that's pretty tall, then set the slideshow section size to Adapt to first image. Check on mobile and you could get stuck on the image itself moving just a little bit: video

Why are these changes introduced?

Fixes #3454

What approach did you take?

Prevent vertical overflow.
That wouldn't have worked on sliders as they can have shadow and borders but the slideshow doesn't so I believe it should be safe.

Visual impact on existing themes

Should fix a mobile issue.

Testing steps/scenarios

Demo links

Checklist

Copy link
Contributor

@Roi-Arthur Roi-Arthur left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems good to me; must admit it took me a while to replicate so wouldn't think it was a massive issue, but broken windows are meant to be fixed 👍🏼

Copy link
Contributor

@sofiamatulis sofiamatulis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Havent been able to reproduce 🤔 Am I missing a step?

https://screenshot.click/08-07-oaj8l-uh8vo.mp4

@ludoboludo
Copy link
Contributor Author

ludoboludo commented May 9, 2024

Havent been able to reproduce 🤔 Am I missing a step?

https://screenshot.click/08-07-oaj8l-uh8vo.mp4

A couple things to try and replicate @sofiamatulis :

  • you'd need to remove the css property i'm adding in the PR but in the code not in the inspect tool
  • and another thing would be to test via the mobile version of the inspect tool instead of the editor (screenshot)

@ludoboludo ludoboludo merged commit c67c833 into main May 9, 2024
8 checks passed
@ludoboludo ludoboludo deleted the fix-slideshow-scroll-issue branch May 9, 2024 14:36
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.

Slideshow scroll issue on mobile with tall images
3 participants