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(slider): range slider thumb on all touch-enabled devices now follows touch gesture #6553

Conversation

Elijbet
Copy link
Contributor

@Elijbet Elijbet commented Mar 2, 2023

Related Issue: #4290

Summary

Replicated the issue on a PC tablet (Dell Precision 5560 per use case) and noticed that the slider was being blocked by the browser swipe gesture. Happened on several browsers tested on this device (Chrome 110.0.5481, MicrosoftEdge 11.0.1587).

Chrome.110.0.5481.mp4

Solved by disabling browser swipe navigation to next/previous page to prevent interference with the slide thumb intended functionality to follow touch gesture. The fix will apply to all touch-enabled devices, including iOS and and tablets running other operating systems.

Can't see a way to automatically test it in our setup, maybe we could add a wiki or confluence page for the slider behavior on other devices (any slider part should use touch-action: none because ...).

@github-actions github-actions bot added the bug Bug reports for broken functionality. Issues should include a reproduction of the bug. label Mar 2, 2023
@Elijbet Elijbet marked this pull request as ready for review March 2, 2023 04:35
@Elijbet Elijbet requested a review from a team as a code owner March 2, 2023 04:35
@Elijbet Elijbet added the pr ready for visual snapshots Adding this label will run visual snapshot testing. label Mar 2, 2023
@Elijbet Elijbet requested review from benelan and jcfranco March 2, 2023 04:45
Copy link
Member

@jcfranco jcfranco left a comment

Choose a reason for hiding this comment

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

Aside from a few comments, this LGTM!

src/components/slider/slider.scss Outdated Show resolved Hide resolved
src/components/slider/slider.scss Outdated Show resolved Hide resolved
@jcfranco
Copy link
Member

jcfranco commented Mar 2, 2023

BTW, I think this fix also applies to iOS devices. Could you revisit the PR title to convey this (see #4845 as an example)?

@Elijbet Elijbet changed the title fix(slider): range slider thumb on PC tablet now follows touch gesture fix(slider): range slider thumb on all touch-enabled devices now follows touch gesture Mar 2, 2023
@Elijbet Elijbet added pr ready for visual snapshots Adding this label will run visual snapshot testing. and removed pr ready for visual snapshots Adding this label will run visual snapshot testing. labels Mar 2, 2023
@Elijbet Elijbet merged commit 70cade7 into master Mar 2, 2023
@Elijbet Elijbet deleted the elijbet/4290-range-slider-thumb-on-PC-tablet-to-follow-touch-gesture branch March 2, 2023 23:55
benelan added a commit that referenced this pull request Mar 7, 2023
…tory

* origin/master: (57 commits)
  ci(eslint): ignore private/internal code for jsdoc rules (#6416)
  fix(modal): ensure modal transitions are in sync (#6564)
  fix(action): ensure consistent width to accommodate indicator when displaying text (#6562)
  build(deps): Bump focus-trap from 7.2.0 to 7.3.1 (#6540)
  feat(block): add built-in localization (#6503)
  revert(stepper-item): emits calciteStepperItemSelect event when selected (#6560)
  refactor: move ref prop last to ensure ref node is in sync (#6530)
  feat(stepper-item): emits `calciteStepperItemSelect` event when selected. (#6521)
  build(deps): Bump @storybook/addon-a11y from 6.5.15 to 6.5.16 (#6539)
  build(deps): Bump eslint from 8.30.0 to 8.35.0 (#6543)
  chore(block): add t9n message bundles. (#6559)
  build: ensure required files are available for doc preview build (#6557)
  fix(slider): range slider thumb on all touch-enabled devices now follows touch gesture (#6553)
  feat(modal): provides `content-top` and `content-bottom` slots (#6490)
  chore(release): 1.0.8
  chore(release): 1.0.8-next.4
  fix(filter, list): filter properly on initialization (#6551)
  chore(release): 1.0.8-next.3
  fix: apply offsetParent polyfill for Chrome 109+ (#6520)
  fix(tree): restore wrapping in tree-item text content (#6518)
  ...
benelan added a commit that referenced this pull request Mar 13, 2023
…efox-esr

* origin/master: (62 commits)
  build(deps): Bump rimraf and @types/rimraf (#6541)
  build(deps): Bump quicktype-core from 6.1.0 to 23.0.12 (#6573)
  test(tabs): delay story due to potential timing chromatic diff (#6437)
  build(deps): Bump chromatic from 6.14.0 to 6.17.1 (#6571)
  build(deps): Bump postcss from 8.4.20 to 8.4.21 (#6570)
  build(deps): Bump eslint-config-prettier from 8.6.0 to 8.7.0 (#6572)
  ci(eslint): ignore private/internal code for jsdoc rules (#6416)
  fix(modal): ensure modal transitions are in sync (#6564)
  fix(action): ensure consistent width to accommodate indicator when displaying text (#6562)
  build(deps): Bump focus-trap from 7.2.0 to 7.3.1 (#6540)
  feat(block): add built-in localization (#6503)
  revert(stepper-item): emits calciteStepperItemSelect event when selected (#6560)
  refactor: move ref prop last to ensure ref node is in sync (#6530)
  feat(stepper-item): emits `calciteStepperItemSelect` event when selected. (#6521)
  build(deps): Bump @storybook/addon-a11y from 6.5.15 to 6.5.16 (#6539)
  build(deps): Bump eslint from 8.30.0 to 8.35.0 (#6543)
  chore(block): add t9n message bundles. (#6559)
  build: ensure required files are available for doc preview build (#6557)
  fix(slider): range slider thumb on all touch-enabled devices now follows touch gesture (#6553)
  feat(modal): provides `content-top` and `content-bottom` slots (#6490)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Bug reports for broken functionality. Issues should include a reproduction of the bug. pr ready for visual snapshots Adding this label will run visual snapshot testing.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants