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 dropdown scroll problem on touch devices #2396

Conversation

mictro
Copy link
Contributor

@mictro mictro commented Jul 25, 2022

Which issue does this PR close?

This PR closes #2335

What is the new behavior?

Prevent page refresh when dropdown is scrolled on touch devices

Does this PR introduce a breaking change?

  • Yes
  • No

Are there any additional context?

Checklist:

The following tasks should be carried out in sequence in order to follow the process of contributing correctly.

Reminders

  • Make sure you have implemented tests following the guidelines in: "The good: Test".
  • Make sure you have updated the cookbook with examples and showcases (for bug fixes, enhancements & new components).

Review

  • Do a self-review.
  • Request that the changes are code-reviewed
  • Request that the changes are UX reviewed (only necessary if your PR introduces visual changes)

When the pull request has been approved it will be merged to develop by Team Kirby.

@mictro mictro requested review from jkaltoft and RasmusKjeldgaard and removed request for jkaltoft July 25, 2022 10:39
@github-actions github-actions bot temporarily deployed to pr-2335-scrolling-swiping-in-dropdown-popout-list-on-native-devices-with-pull-to-refresh July 25, 2022 10:43 Inactive
Copy link
Collaborator

@RasmusKjeldgaard RasmusKjeldgaard left a comment

Choose a reason for hiding this comment

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

Nice fix, I have proposed a minor refactor. Let me know what you think.

Comment on lines 239 to 242
onTouchStart(event: TouchEvent): void {
// Prevent touchmove propgation to allow for scroll on page with pull-to-refresh;
event.stopPropagation();
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Consider replacing this with the following snippet.

I think it should result in identical behavior, and it uses a similar pattern to how we have done it for the keydown events in this component (bottom of this file). As an added bonus we should not need to add anything to the template.

@HostListener('touchstart', ['$event']) _onTouchStart(event: TouchEvent) {
  event.stopPropagation();
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

@mictro mictro force-pushed the bug/2335-scrolling-swiping-in-dropdown-popout-list-on-native-devices-with-pull-to-refresh branch from c6e8346 to 0d5f9c6 Compare August 2, 2022 11:41
@mictro mictro requested a review from RasmusKjeldgaard August 2, 2022 11:46
@github-actions github-actions bot temporarily deployed to pr-2335-scrolling-swiping-in-dropdown-popout-list-on-native-devices-with-pull-to-refresh August 2, 2022 11:46 Inactive
Copy link
Collaborator

@RasmusKjeldgaard RasmusKjeldgaard left a comment

Choose a reason for hiding this comment

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

Very nice 👍

…pout-list-on-native-devices-with-pull-to-refresh
@mictro mictro merged commit 32c47d2 into develop Aug 4, 2022
@mictro mictro deleted the bug/2335-scrolling-swiping-in-dropdown-popout-list-on-native-devices-with-pull-to-refresh branch August 4, 2022 07:08
This was referenced Aug 11, 2022
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.

[BUG] Scrolling/swiping in dropdown on native devices with pull to refresh
2 participants