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

feat(modal): swipe modal from content #21227

Closed
wants to merge 5 commits into from
Closed

feat(modal): swipe modal from content #21227

wants to merge 5 commits into from

Conversation

julian-baumann
Copy link
Contributor

@julian-baumann julian-baumann commented May 6, 2020

Pull request checklist

Please check if your PR fulfills the following requirements:

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been reviewed and added / updated if needed (for bug fixes / features)
  • Build (npm run build) was run locally and any changes were pushed
  • Lint (npm run lint) has passed locally and any fixes were made for failures

Pull request type

Please check the type of change your PR introduces:

  • Bugfix
  • Feature
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • Documentation content changes
  • Other (please describe):

What is the current behavior?

Modal can dragged from every component except the ion-content.

resolves #22046

What is the new behavior?

ezgif com-video-to-gif-4

Now it can also be dragged by the ion-content.

Does this introduce a breaking change?

  • Yes
  • No

Other information

Adds functionality to swipe the modal not just from the header but also from the content, if it's scrolled to the top.
@ionitron-bot ionitron-bot bot added the package: core @ionic/core package label May 6, 2020
Copy link
Contributor

@liamdebeasi liamdebeasi left a comment

Choose a reason for hiding this comment

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

Is there any way we can do this without interrupting/hijacking scrolling in the modal? I understand this behavior is something native iOS does, but I am concerned that this may have undesired side effects.

@julian-baumann
Copy link
Contributor Author

@liamdebeasi What exactly do you mean with "interrupting/hijacking"? I tested it for a couple of weeks now and it seems pretty reliable. To my surprise it also works very well with the iOS over-scroll effect.

@liamdebeasi
Copy link
Contributor

Doing stuff like this: https://github.com/ionic-team/ionic/pull/21227/files#diff-aa4516c348b6917700ac14e788174be0R62-R65

Setting overflow: hidden is going to prevent users from scrolling on the modal. We typically try to stay away from anything that is going to prevent users from scrolling on content (with a few exceptions).

@julian-baumann
Copy link
Contributor Author

julian-baumann commented May 19, 2020

Ah I see. Even though it should be always possible to scroll, because overflow is set to "auto" every time the gesture ends or starts, I can understand your argument very well. May you have any suggestions on doing it otherwise?

@roman-rr
Copy link

@julian-baumann
There are also can have conflicts with any horizontal drag components like ion-item-options, or swiper, be sure you check and handle it.

But you can realize it like this:

  1. Check if scrollTop is null
  2. If null allow drag to bottom direction
  3. if scrollTop > 0 disable bottom drag, enable scroll drag

Otherwise i recommend for you using https://github.com/roman-rr/cupertino-pane
Or check how it implemented there.
If you need 3D effect it will be soon implemented.

@julian-baumann
Copy link
Contributor Author

@roman-rr The modal handles components like the ion-item-options very well, just checked it. And to your other suggestions; I think that's exactly what I did there, but also the content has to be stoped from scrolling because otherwise it would behave strange with the over-scroll effect on iOS devices... That's why overflow: hidden is used.

@liamdebeasi
Copy link
Contributor

Hi there,

Is there any way this can be accomplished without manually setting the overflow value on the content? This is a bit too risky of a change for me.

@roman-rr
Copy link

roman-rr commented Jun 23, 2020

This is can be accomplished without changing overflow value if modal has only two position (top and bottom).

Just prevent scroll gesture on user drag event if scrollTop === 0 and drag event towards the bottom;
Note: other element with drag gestures can be moved in one moment with modal, that why good idea to define draggable angle for pane ~45 Degree.

@julian-baumann
Copy link
Contributor Author

julian-baumann commented Jun 23, 2020

Sorry, I don't really get your train of thought😁
How exactly do you want to "prevent the scroll gesture"? May you leave a quick code snipped or something?

Also checked your cupertino pane. Nice work.

@roman-rr
Copy link

roman-rr commented Jun 23, 2020

@julian-baumann
Thank you!

Something like this:

private contentScrollTop: number = 0;

function touchmoveEvent(event) {
  // Element with overflow
  this.overflowEl.addEventListener('scroll', (s: any) => {
    this.contentScrollTop = s.target.scrollTop;
  });

  // Scrolled -> Prevent touchmoveEvent
  if (this.contentScrollTop > 0) {
    return;
  }
  
  // Continue touch move event
  ...
}

Don't sure how to implement into ionic, need make some core investigations.

@liamdebeasi
Copy link
Contributor

We want to avoid doing things like: scrollElement.style.overflow = hidden; as they can cause undesirable side effects.

I am comfortable having a scroll listener as long as a) It is cleaned up when the component is destroyed and b) we are not doing computationally expensive operations in the callback.

In other words, scrolling is really smooth in the modal currently, and I do not want that to change 😄. Thanks for all of your hard work so far!

@laocker96
Copy link

Sorry this feature is still not working for me and my code is something like this #21612, really simple. Is this working now or it's still being fixed?

@bushev
Copy link

bushev commented Jul 3, 2021

Hey guys, that's the plan on merging this?

@liamdebeasi
Copy link
Contributor

As I mentioned in #21227 (comment), I was hoping the PR author could accomplish this functionality without interrupting scrolling or in a way that is less likely to have scrolling jank. Without one of those, it is unlikely we will merge this PR.

@BerkeAras
Copy link

@julian-baumann Any updates?

@julian-baumann
Copy link
Contributor Author

@julian-baumann Any updates?

Haven't looked into this a long time.
I don't know any other (less janky) way from interrupting the content to scroll other than using overflow: hidden. Without setting this, the content underneath also scrolls when the upper layer (the whole modal dialog) is being dragged down, which results in a very unnatural feeling. Not the way Apple implemented it natively.

I'm open to ideas. @liamdebeasi may you have some?

@liamdebeasi
Copy link
Contributor

Thanks for the PR. We appreciate all the work you put into creating this. I am going to close this in favor of #25185.

I will give you co-author credit when the PR is merged into our main code base. Please see #22046 (comment) for a dev build if you would like to test this in your application.

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

Successfully merging this pull request may close these issues.

feat: swipe to close modal from content
6 participants