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

Various components: Prevent panning page by touch #8394

Merged
merged 2 commits into from
Apr 16, 2024

Conversation

danielchalmers
Copy link
Contributor

@danielchalmers danielchalmers commented Mar 18, 2024

Description

Resolves #8379
Resolves #4396

I don't know the full ramifications on blanket applying touch-action: none on popover descendants.

In the future this could be added to controls like tabs and rating controls to implement swipe functionality.

How Has This Been Tested?

visually

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
timepicker.mp4
snackbar.mp4
slider.mp4
popover.mp4
menu.mp4
colorpicker.mp4

Checklist:

  • The PR is submitted to the correct branch (dev).
  • My code follows the code style of this project.
  • I've added relevant tests.

This is a demonstration for MudBlazor#8379

I don't know the full ramifications on blanket applying touch-action:none on popover descendents.

Future considerations: Enable swiping in tabs and rating controls which would warrant touch-action:none on them as well.
@github-actions github-actions bot added bug Something does not work as intended/expected PR: needs review labels Mar 18, 2024
@danielchalmers
Copy link
Contributor Author

@Garderoben I put together a demonstration of the issue mentioned in #8336 (comment) and my proposed fix so you can see it in action

@danielchalmers
Copy link
Contributor Author

This is after. Find before at #8379 mentioned above.

This PR brings MudBlazor closer to MUI in this regard.

Copy link

codecov bot commented Mar 18, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.60%. Comparing base (1df0d8d) to head (1336e9e).
Report is 95 commits behind head on dev.

Additional details and impacted files
@@            Coverage Diff             @@
##              dev    #8394      +/-   ##
==========================================
+ Coverage   88.92%   89.60%   +0.68%     
==========================================
  Files         410      420      +10     
  Lines       12253    14317    +2064     
  Branches     2452     3110     +658     
==========================================
+ Hits        10896    12829    +1933     
- Misses        826      908      +82     
- Partials      531      580      +49     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@danielchalmers
Copy link
Contributor Author

This a precursor to #6022 #7203 which I plan on taking a look at afterwards
End goal: https://mui.com/x/react-date-pickers/time-picker/

@Garderoben Garderoben self-requested a review March 19, 2024 17:13
@danielchalmers

This comment was marked as off-topic.

@danielchalmers
Copy link
Contributor Author

One method is to disable scrolling altogether when the popover is open.

Ideally it would only prevent scrolling when the cursor/touch is physically inside the popover. That is a little harder to implement and will probably require javascript.

The method in this PR prevents scrolling/panning (and zoom) with touch events but not the cursor. This is fine and is the simplest fix.

@danielchalmers danielchalmers mentioned this pull request Mar 24, 2024
6 tasks
@danielchalmers danielchalmers deleted the touch-behind branch April 4, 2024 23:46
@danielchalmers danielchalmers restored the touch-behind branch April 4, 2024 23:46
@danielchalmers danielchalmers reopened this Apr 4, 2024
danielchalmers added a commit to danielchalmers/MudBlazor that referenced this pull request Apr 5, 2024
- Simplifies the code
- Makes the color picker fluid
- Adds configurable debounce
- Fixes some niche issues like svg selection

Touch support will come later after MudBlazor#8394 is resolved
@danielchalmers danielchalmers mentioned this pull request Apr 5, 2024
6 tasks
@Garderoben
Copy link
Member

@MudBlazor/core-team opinions about this?

I'm only worried about one thing and that's the use of the asterisk selector in the popover. It can drag down css load time. However i might be worried over nothing. My suggestion would be to add the necessary CSS changes to our own pickers themself where needed but this would not fix it for people building their own components in our popover.

@Garderoben Garderoben merged commit 7f5c076 into MudBlazor:dev Apr 16, 2024
5 checks passed
@kelltom
Copy link

kelltom commented Apr 16, 2024

I'd also really like to see something like this get implemented. MudBlazor has lots of potential for mobile applications.

@danielchalmers
Copy link
Contributor Author

I'd also really like to see something like this get implemented. MudBlazor has lots of potential for mobile applications.

@kelltom Do you have any other components in mind that are lacking in this area? I missed the static pickers for one.

@kelltom
Copy link

kelltom commented Apr 16, 2024

@danielchalmers Ones that come to mind:

  • What brought me here were the Date and Time pickers, as they have a mobile-friendly appearance but aren't touch-friendly.
  • MudListItems could have a swipe event to expose an action button (e.g., swipe to delete).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something does not work as intended/expected PR: needs review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Various components: Prevent panning page by touch Scrolling should not cause slider to move
3 participants