fix(VSlideGroup): detect horizontal/vertical swipe intent #13005

yuwu9145 · 2021-01-24T01:45:22Z

partically fixes #10673

Description

As per suggestions in #10673, VSlideGroup should determine swipe intent (horizontal or vertical) first, then using swipe intent to only allow one of horizontal/vertical swipe gestures between each onTouchStart and onTouchEnd session

fixes #12293

partically fixes #10673

Motivation and Context

fixes #12293

partically fixes #10673

How Has This Been Tested?

visually

Markup:

// Paste your FULL Playground.vue here
<template>
  <v-container>
    <v-slide-group>
      <v-slide-item v-for="i in 10" :key="i" class="ma-5">
        <v-card color="#eeeeee" elevation="10" max-width="300px">
          <v-img
            src="https://cdn.vuetifyjs.com/images/cards/sunshine.jpg"
            height="200px"
          ></v-img>
          <v-card-text>
            SWITCH TO MOBILE
            <v-divider></v-divider>
            Try swiping up/down while still keeping finger on these slides
          </v-card-text>
          <v-card-actions>
            <v-spacer />
            <v-btn text> Go </v-btn>
          </v-card-actions>
        </v-card>
      </v-slide-item>
    </v-slide-group>
    <v-card class="ma-2 pa-5" v-for="i in 40" :key="i">
      <v-content>You cannot scroll past the above slides on mobile</v-content>
    </v-card>
  </v-container>
</template>

<script>
export default {
  data() {
    return {
    };
  },
};
</script>

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)
Improvement/refactoring (non-breaking change that doesn't add any features but makes things better)

Checklist:

The PR title is no longer than 64 characters.
The PR is submitted to the correct branch (master for bug fixes and documentation updates, dev for new features and backwards compatible changes and next for non-backwards compatible changes).
My code follows the code style of this project.
I've added relevant changes to the documentation (applies to new features and breaking changes in core library)

fixes vuetifyjs#12293/npartically fixes vuetifyjs#10673

yuwu9145 · 2021-01-24T03:31:46Z

packages/vuetify/src/components/VSlideGroup/VSlideGroup.ts

+        // sliding horizontally
+        this.scrollOffset = this.startX - e.touchmoveX
+        // temporarily disable window vertical scrolling
+        document.documentElement.style.overflowY = 'hidden'


When swiping horizontally, I tried to achieve disabling vertical scrolling or touch swipe via css by setting touch-action to pan-x. However, touch-action only takes effect from next touch swipe, therefore changing touch-action to pan-x during onTouchMove will not restrict current touch move to "horizontal only" but has to wait until next touch action starts.

Hence I use a javascript way that disables whole page's vertical scroll if swiping horizontally is determined. (And remove this style in touchEnd event triggered in same component VSlideGroup). Please advice if there is a better way to disable vertical scroll passthrough once horizontal swipe intent is detected

johnleider · 2021-02-18T22:35:12Z

@babyraging can you reproduce your issue in #11571 with this?

yuwu9145 · 2021-03-03T04:22:52Z

@babyraging can you reproduce your issue in #11571 with this?

Hi @johnleider , I did a test simulating @babyraging issue in #11571, this pull request does resolve it.

This is the mock that I used to test:

<template>
  <v-container>
      <v-chip-group
          mandatory
          column
          active-class="primary--text"
        >
          <v-chip
            v-for="tag in tags"
            :key="tag"
          >
            {{ tag }}
          </v-chip>
        </v-chip-group>

  </v-container>
</template>

<script>
export default {
  data() {
    return {
    tags: [
        'Work',
        'Home Improvement',
        'Vacation',
        'Food',
        'Drawers',
        'Shopping',
        'Art',
        'Tech',
        'Creative Writing',
        'Shopping',
        'Art',
        'Tech',
        'Shopping',
        'Art',
        'Tech',
        'Shopping',
        'Art',
        'Tech',
        'Shopping',
        'Art',
        'Tech',
        'Work',
        'Home Improvement',
        'Vacation',
        'Food',
        'Drawers',
        'Shopping',
        'Art',
        'Tech',
        'Creative Writing',
        'Shopping',
        'Art',
        'Tech',
        'Shopping',
        'Art',
        'Tech',
        'Shopping',
        'Art',
        'Tech',
        'Shopping',
        'Art',
        'Tech',
        'Work',
        'Home Improvement',
        'Vacation',
        'Food',
        'Drawers',
        'Shopping',
        'Art',
        'Tech',
        'Creative Writing',
        'Shopping',
        'Art',
        'Tech',
        'Shopping',
        'Art',
        'Tech',
        'Shopping',
        'Art',
        'Tech',
        'Shopping',
        'Art',
        'Tech',
        'Work',
        'Home Improvement',
        'Vacation',
        'Food',
        'Drawers',
        'Shopping',
        'Art',
        'Tech',
        'Creative Writing',
        'Shopping',
        'Art',
        'Tech',
        'Shopping',
        'Art',
        'Tech',
        'Shopping',
        'Art',
        'Tech',
        'Shopping',
        'Art',
        'Tech',
        'Work',
        'Home Improvement',
        'Vacation',
        'Food',
        'Drawers',
        'Shopping',
        'Art',
        'Tech',
        'Creative Writing',
        'Shopping',
        'Art',
        'Tech',
        'Shopping',
        'Art',
        'Tech',
        'Shopping',
        'Art',
        'Tech',
        'Shopping',
        'Art',
        'Tech',
        'Work',
        'Home Improvement',
        'Vacation',
        'Food',
        'Drawers',
        'Shopping',
        'Art',
        'Tech',
        'Creative Writing',
        'Shopping',
        'Art',
        'Tech',
        'Shopping',
        'Art',
        'Tech',
        'Shopping',
        'Art',
        'Tech',
        'Shopping',
        'Art',
        'Tech',
      ],
    };
  }
};
</script>

Also, @babyraging please feel free to leave any comments

HammerMeetNail · 2021-03-05T03:41:36Z

Eagerly awaiting this fix 🙏

Recently deployed a few slides on a single page and it’s very difficult to scroll the page on mobile. If you need someone to try this out to confirm it fixes vertical scroll on mobile I should be able to verify on my site tomorrow night.

packages/vuetify/src/components/VSlideGroup/VSlideGroup.ts

johnleider

I'll take a look at the globals issue. You won't be able to import using the @ syntax.

packages/vuetify/src/components/VSlideGroup/VSlideGroup.ts

yuwu9145 · 2021-03-11T00:41:47Z

I'll take a look at the globals issue. You won't be able to import using the @ syntax.

Hi @johnleider , just want to clarify, should I add import { IN_BROWSER } from '../../../../docs/src/util/globals' into VSliderGroup? Feeling it is pretty hecky though (no component currently does this)

HammerMeetNail · 2021-03-18T01:00:41Z

Hi all - super appreciative of the work you're all doing. Looking to give this a bump cause it should make our site a lot more usable on mobile.

yuwu9145 · 2021-03-18T02:12:54Z

Hi all - super appreciative of the work you're all doing. Looking to give this a bump cause it should make our site a lot more usable on mobile.

Would you mind helping to verify if this fix resolves your website issue? I am happy to give you a guidance of how to do it if you need help ;)

HammerMeetNail · 2021-03-18T03:21:06Z

Absolutely, I'll have time tomorrow night EDT. I'll give you a shout if I have any trouble.

HammerMeetNail · 2021-03-18T23:09:47Z

@yuwu9145 I can confirm this fixes our slide group issue. Our page looks roughly like this:

<template>
  <div class="d-flex flex-grow-1 flex-column">

    <!-- First Slide Group-->
    <header>Header1</header>
    <v-row class="flex-grow-0" dense>
      <v-col cols="12">
        <slide :stuff="stuff"></slide>
      </v-col>
    </v-row>
    
    <!-- Second Slide Group-->
    <header>Header2</header>
    <v-row class="flex-grow-0" dense>
      <v-col cols="12">
        <slide :stuff="stuff"></slide>
      </v-col>
    </v-row>
    
    <!-- Third Slide Group-->
    <header>Header3</header>
    <v-row class="flex-grow-0" dense>
      <v-col cols="12">
        <slide :stuff="stuff"></slide>
      </v-col>
    </v-row>

  </div>
</template>

Before your fix we'd be able to recreate the issue in Chrome dev tools with the following steps:

Open inspector
Switch to mobile view
Try and click an item in the slide group and drag (just like you'd do if you were using touch)

This normally would prevent vertical scroll. With your fix, we can click any item in the slide group and scroll vertically.

Very eager to see this PR touched up and merged :)

yuwu9145 · 2021-03-18T23:43:53Z

@HammerMeetNail Thanks a lot!

…13005)

fix(VSlideGroup): detect horizontal/vertical swipe intent

9bd7087

fixes vuetifyjs#12293/npartically fixes vuetifyjs#10673

yuwu9145 marked this pull request as ready for review January 24, 2021 01:47

yuwu9145 marked this pull request as draft January 24, 2021 02:01

temporarily disalbe page vertical scrolling when swipe horizontal

174afcf

yuwu9145 commented Jan 24, 2021

View reviewed changes

yuwu9145 marked this pull request as ready for review January 24, 2021 03:35

johnleider requested a review from a team February 9, 2021 20:00

johnleider assigned yuwu9145 Feb 9, 2021

johnleider added C: VSlideGroup T: bug labels Feb 9, 2021

johnleider added this to the v2.4.x milestone Feb 9, 2021

KaelWD force-pushed the master branch 12 times, most recently from ddbea7b to c7a93f7 Compare February 19, 2021 14:19

johnleider requested changes Mar 6, 2021

View reviewed changes

packages/vuetify/src/components/VSlideGroup/VSlideGroup.ts Show resolved Hide resolved

packages/vuetify/src/components/VSlideGroup/VSlideGroup.ts Show resolved Hide resolved

packages/vuetify/src/components/VSlideGroup/VSlideGroup.ts Show resolved Hide resolved

Address pull request feedback

3bd6aa1

johnleider requested changes Mar 8, 2021

View reviewed changes

packages/vuetify/src/components/VSlideGroup/VSlideGroup.ts Outdated Show resolved Hide resolved

Remove comment as per requested

f60bb78

This comment has been minimized.

Sign in to view

johnleider approved these changes Mar 31, 2021

View reviewed changes

johnleider requested review from KaelWD and nekosaur March 31, 2021 21:19

johnleider approved these changes Apr 6, 2021

View reviewed changes

johnleider merged commit f6124be into vuetifyjs:master Apr 6, 2021

Malachi-M pushed a commit to kapost/vuetify that referenced this pull request Apr 22, 2021

fix(VSlideGroup): detect horizontal/vertical swipe intent (vuetifyjs#…

8639fdf

…13005)

vuetifyjs deleted a comment Apr 30, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

fix(VSlideGroup): detect horizontal/vertical swipe intent #13005

fix(VSlideGroup): detect horizontal/vertical swipe intent #13005

yuwu9145 commented Jan 24, 2021 •

edited

Loading

Uh oh!

yuwu9145 Jan 24, 2021 •

edited

Loading

Uh oh!

johnleider commented Feb 18, 2021

Uh oh!

yuwu9145 commented Mar 3, 2021

Uh oh!

HammerMeetNail commented Mar 5, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

johnleider left a comment

Uh oh!

Uh oh!

yuwu9145 commented Mar 11, 2021 •

edited

Loading

Uh oh!

HammerMeetNail commented Mar 18, 2021

Uh oh!

yuwu9145 commented Mar 18, 2021

Uh oh!

HammerMeetNail commented Mar 18, 2021

Uh oh!

HammerMeetNail commented Mar 18, 2021

Uh oh!

yuwu9145 commented Mar 18, 2021

Uh oh!

This comment has been minimized.

fix(VSlideGroup): detect horizontal/vertical swipe intent #13005

fix(VSlideGroup): detect horizontal/vertical swipe intent #13005

Conversation

yuwu9145 commented Jan 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Motivation and Context

How Has This Been Tested?

Markup:

Types of changes

Checklist:

Uh oh!

yuwu9145 Jan 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

johnleider commented Feb 18, 2021

Uh oh!

yuwu9145 commented Mar 3, 2021

Uh oh!

HammerMeetNail commented Mar 5, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

johnleider left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

yuwu9145 commented Mar 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

HammerMeetNail commented Mar 18, 2021

Uh oh!

yuwu9145 commented Mar 18, 2021

Uh oh!

HammerMeetNail commented Mar 18, 2021

Uh oh!

HammerMeetNail commented Mar 18, 2021

Uh oh!

yuwu9145 commented Mar 18, 2021

Uh oh!

This comment has been minimized.

yuwu9145 commented Jan 24, 2021 •

edited

Loading

yuwu9145 Jan 24, 2021 •

edited

Loading

yuwu9145 commented Mar 11, 2021 •

edited

Loading