Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

feat(modal): add expandToScroll property to allow scrolling at all breakpoints #30097

Merged
merged 45 commits into from
Feb 3, 2025

Conversation

kumibrr
Copy link

@kumibrr kumibrr commented Dec 22, 2024

Issue number: resolves #24631


What is the current behavior?

Sheet modals do not allow content scrolling unless its breakpoint is 1.

What is the new behavior?

Sheet modals now have an additional property expandToScroll which allows the sheet modal to scroll content at all breakpoints when it is set to false.

Does this introduce a breaking change?

  • Yes
  • No

Other information

Screen.Recording.2024-12-23.at.00.41.19.mov

@kumibrr kumibrr requested a review from a team as a code owner December 22, 2024 23:37
Copy link

vercel bot commented Dec 22, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
ionic-framework 🛑 Canceled (Inspect) Feb 1, 2025 0:06am

@github-actions github-actions bot added the package: core @ionic/core package label Dec 22, 2024
@davidalejandroaguilar
Copy link

I'm new to Ionic and I'm excited to see this closing a much needed 3 year old issue. Happy to be here just in time! 🥳

Looking forward to implementing these awesome UX patterns https://sarunw.com/posts/bottom-sheet-in-ios-15-with-uisheetpresentationcontroller/

@thetaPC
Copy link
Contributor

thetaPC commented Jan 9, 2025

@kumibrr Thank you for submitting this PR!

Could you explain why a new array, snapBreakpoints, is being created instead of just using the existing breakpoints array? If we continue using breakpoints then it would follow the setup on native iOS.

@kumibrr
Copy link
Author

kumibrr commented Jan 9, 2025

@kumibrr Thank you for submitting this PR!

Could you explain why a new array, snapBreakpoints, is being created instead of just using the existing breakpoints array? If we continue using breakpoints then it would follow the setup on native iOS.

@thetaPC Thank you for reviewing and asking!

There's two main reasons I made a new array, the first is to avoid breaking changes with any current setup.
The second one is that there's some exceptions in the native iOS setup, like on Maps, in which there's some breakpoints that do not allow content scroll despite having scrollable content.

ScreenRecording_01-09-2025.23-24-59_1.MP4

Having a separate array enables developers to have more control in which breakpoints they want to enable scrolling.
Attaching this behavior to the breakpoints array would remove that flexibility.

@thetaPC
Copy link
Contributor

thetaPC commented Jan 10, 2025

@kumibrr That makes sense to me. Could you try a video that showcases your proposed behavior? Or provide steps on how I can view it on my end? It seems that the one you sent in the last comment is not rendering.

@thetaPC thetaPC self-requested a review January 10, 2025 01:00
@kumibrr
Copy link
Author

kumibrr commented Jan 10, 2025

@thetaPC Sure, sorry about that.
Here's two videos, one in the Maps app, where I showcase the current native behavior, and the proposed one in the preview url, in which breakpoints = [0,0.25, 0.5, 0.75] and snapBreakpoints = [0.5, 0.75].

If videos do not work, you can try the proposed behavior in the following link, then clicking on "Present Sheet Modal (snapBreakpoints)", it has breakpoints = [0,0.25, 0.5, 0.75] and snapBreakpoints = [0.5, 0.75] https://ionic-framework-git-fork-kumibrr-main-ionic1.vercel.app/src/components/modal/test/sheet

Screen.Recording.2025-01-10.at.09.29.11.mp4
Screen.Recording.2025-01-10.at.09.26.56.mp4

@thetaPC
Copy link
Contributor

thetaPC commented Jan 10, 2025

@kumibrr thank you for the information!!

I should have been more clear though, apologies. I'm trying to get an example of a native app that shows the behavior of the snap breakpoints that you're proposing. iOS Maps only shows the scroll possible when the sheet is fully revealed. I would like to see a native app that supports scroll at certain breakpoints but not all of them. For example, scroll is possible on 100% and 50% but not on 75%.

I'm not against introducing snap breakpoints but we need a native case to support adding it in.

@kumibrr
Copy link
Author

kumibrr commented Jan 10, 2025

@thetaPC That's something I thought when making it, as you said, I could not find any native app that matches this.

I also had in mind scrollThreshold instead of snapBreakpoints, which would receive a single breakpoint, and would make any breakpoints above it scrollable.
ex. scrollThreshold="0.5".
It matches native behavior much better, though not as flexible.

If that sounds good, should I make a new PR with the alternative scrollThreshold and close this one? Or should I push new commits to this one and change the title?

@thetaPC
Copy link
Contributor

thetaPC commented Jan 11, 2025

Understood! I'll have to talk to the team to decide on the approach. The goal is to stay as close to native as possible so that might lead to sticking with an all or nothing concept like iOS: either scrolling only happens on max height or scrolling happens in all heights.

I'll aim to get back to you within the next week since this PR is very promising!

@thetaPC
Copy link
Contributor

thetaPC commented Jan 14, 2025

@kumibrr hope your weekend went well!

I've discussed it with the team and we want to stick with how native iOS does it, meaning that either the user can scroll when the sheet is fully expanded or at all the breakpoints. This can be done with a boolean. Here's an example of the direction that we want to go. We would like for this PR to use the boolean and also add/update comments (again examples can be shown via the link). We are looking forward to seeing those requested changes!

Copy link
Contributor

@thetaPC thetaPC left a comment

Choose a reason for hiding this comment

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

As mentioned in our previous conversation, let's use a boolean to determine when the scrolling will happen.

@kumibrr
Copy link
Author

kumibrr commented Jan 14, 2025

@thetaPC Fine by me!
I've just submitted the requested changes, but the added footer to the modal content appears abruptly after the modal has opened.

@kumibrr
Copy link
Author

kumibrr commented Jan 14, 2025

@thetaPC I've done some more changes to fix the footer animation on creation. Now the footer appears with the content, no jittering.

I have also moved the contentAnimation initialization from modal/gestures/sheet.ts to modal/animations/sheet.ts where it belongs.

And I've added a new property animateContentHeight in ModalAnimationOptions that will enable the contentAnimation when scrollAtEdge === false

EDIT: Just tested in Safari and the footer jumps a bit during the contentAnimation in moveSheetToBreakpoint. I'll fix it later.

@kumibrr
Copy link
Author

kumibrr commented Jan 14, 2025

Alright. I've spent quite some time but couldn't figure out how to fix it. This also happens in mobile Safari, mobile Chrome (using an Android device) but Chrome desktop masked it (it changes so fast I didn't caught it)

As seen in the attached gif, the problem is that the footer jumps when breakpoint changes. I have my guesses, but I don't want to bias any guesses.

ScreenRecording2025-01-14at20 10 42-ezgif com-video-to-gif-converter.

@thetaPC
Copy link
Contributor

thetaPC commented Jan 14, 2025

@kumibrr I do see the flicker of the footer on my end. This has been one of the major issues that we encounter when trying to introduce the feature. What are your guesses of why it's happening?

@github-actions github-actions bot added the package: angular @ionic/angular package label Jan 31, 2025
@thetaPC thetaPC changed the title feat(modal): add scrollAtEdge property to allow scrolling at all breakpoints feat(modal): add expandToScroll property to allow scrolling at all breakpoints Jan 31, 2025
thetaPC and others added 7 commits January 31, 2025 16:01
Co-authored-by: Brandy Smith <brandyscarney@users.noreply.github.com>
Co-authored-by: Brandy Smith <brandyscarney@users.noreply.github.com>
Co-authored-by: Brandy Smith <brandyscarney@users.noreply.github.com>
Co-authored-by: Brandy Smith <brandyscarney@users.noreply.github.com>
Co-authored-by: Brandy Smith <brandyscarney@users.noreply.github.com>
Co-authored-by: Brandy Smith <brandyscarney@users.noreply.github.com>
Co-authored-by: Brandy Smith <brandyscarney@users.noreply.github.com>
@thetaPC thetaPC merged commit 166e435 into ionic-team:feature-8.5 Feb 3, 2025
46 checks passed
@thetaPC
Copy link
Contributor

thetaPC commented Feb 3, 2025

@kumibrr Thank you so much for getting this PR going! It's been approved and should be released in 8.5. Congrats to your contribution and for it being your first PR!!

@rgoldiez
Copy link

rgoldiez commented Feb 7, 2025

@thetaPC - any idea on when 8.5 will be released?

@thetaPC
Copy link
Contributor

thetaPC commented Feb 8, 2025

@rgoldiez Hi there! Version 8.5 doesn’t have an official release date yet, but definitely keep an eye out as it will be available soon. Thanks for your patience!

@a7medm7med
Copy link

@kumibrr @thetaPC Thank you so much for the amazing work! This feature was definitely missing in Ionic. However, I believe there’s one more improvement that would enhance the UX. Without breakpoints, the modal can be dismissed by swiping down anywhere inside it. But when using breakpoints, it can only be closed by swiping down on the header, which feels restrictive from a usability perspective. It would be great if swipe-to-dismiss could work consistently across all cases.

@thetaPC
Copy link
Contributor

thetaPC commented Mar 7, 2025

@a7medm7med Thank you for letting us know! We've confirmed that native iOS does allow this and we are working on adding this.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
package: angular @ionic/angular package package: core @ionic/core package package: vue @ionic/vue package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feat: sheet modal, add option to prefer scrolling when not fully expanded
7 participants