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

fix: close reveal only on click-event starting outside modal #11816

Closed
wants to merge 1 commit into from
Closed

fix: close reveal only on click-event starting outside modal #11816

wants to merge 1 commit into from

Conversation

zipzapp
Copy link

@zipzapp zipzapp commented Sep 6, 2019

Description

This pr changes the _events binding on the reveal component to use the mousedown jQuery event instead of the click jQuery event so that modals don't close unless a click event begins outside of a the modal area, fixing the issue linked below.

Types of changes

  • Documentation
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (anything that would change an existing functionality)
  • Maintenance (refactor, code cleaning, development tools...)

Checklist

  • I have read and follow the CONTRIBUTING.md document.
  • The pull request title and template are correctly filled.
  • The pull request targets the right branch (develop or develop-v...).
  • My commits are correctly titled and contain all relevant information.
  • I have updated the documentation accordingly to my changes (if relevant).
  • I have added tests to cover my changes (if relevant).

@DanielRuf
Copy link
Contributor

Hi @zipzapp,

Thanks for your PR. We will check that.

What about tap / touch? I think there might be some issues.

@zipzapp
Copy link
Author

zipzapp commented Sep 6, 2019

@DanielRuf I'm not sure about all devices, but on mobile, the modal takes up the whole screen; so I don't think the issue would be applicable.

@DanielRuf
Copy link
Contributor

It depends on the viewport size.

@joeworkman joeworkman changed the base branch from develop-v6.5 to develop September 10, 2019 15:55
@joeworkman joeworkman changed the base branch from develop to develop-v6.5 September 10, 2019 15:55
@joeworkman
Copy link
Member

Is there anyway that you could resubmit this PR against the develop branch instead of the develop-v6.5?

@DanielRuf
Copy link
Contributor

Let me check if we can change it or rebase it.

@DanielRuf DanielRuf changed the base branch from develop-v6.5 to develop September 10, 2019 16:21
@DanielRuf DanielRuf changed the base branch from develop to develop-v6.5 September 10, 2019 16:21
@DanielRuf
Copy link
Contributor

Ok we have to rebase this PR. I'll do this later.

@DanielRuf DanielRuf changed the base branch from develop-v6.5 to develop September 10, 2019 20:49
@DanielRuf
Copy link
Contributor

I have rebased it.
Please do not add the lockfile next time.

I still think this PR does not solve it. Also develop uses click + tap.

@DanielRuf
Copy link
Contributor

Hi @zipzapp,

Thanks for your PR and contribution to Foundation Sites.

I am closing this as this is not the right solution. We have to check if it is a normal tap or touchmove / swipe and track the position using touchstart / touchend.

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.

Drag from inside modal & ending outside closes modal (Chrome, Safari)
3 participants