-
Notifications
You must be signed in to change notification settings - Fork 201
feat(touch-action): adding touch action none to draggable components #3656
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
base: spectrum-two
Are you sure you want to change the base?
Changes from all commits
fe4cef8
d875a7f
4580ba2
e1a0e42
b8f7b08
295ed1e
8de4da1
8c1ff4f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,12 @@ | ||
--- | ||
"@spectrum-css/colorhandle": minor | ||
"@spectrum-css/colorslider": minor | ||
"@spectrum-css/colorarea": minor | ||
"@spectrum-css/slider": minor | ||
--- | ||
|
||
### Including the touch action rule for draggable content | ||
|
||
The slider, color slider, color area, color wheel, color handle all deserve to have their touch-action property manages so that trying to set the value of those interfaces doesn't cause page scrolling in touch context. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think "manages" should be past tense, "managed." |
||
|
||
Adding `touch-action: none` to a slider or any draggable component is necessary to prevent the browser's default touch behaviors—such as scrolling, pinch-zooming, or double-tap zooming—from interfering with the component's intended interaction. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I also think we can remove the hyphen between "zooming" and "from." |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -207,6 +207,9 @@ | |
padding-inline-end: var(--mod-slider-handle-gap, var(--spectrum-slider-handle-gap)); | ||
margin-inline-start: var(--mod-slider-track-margin-offset, var(--spectrum-slider-track-margin-offset)); | ||
|
||
touch-action: none; | ||
user-select: none; | ||
Comment on lines
+210
to
+211
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You left really great comments in color handle CSS regarding these two properties. Did you want to add them here as well? Not a blocker if you don't feel like it- I just really liked the comments! 😆 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @aramos-adobe I was trying to test this, and struggling with the slider. I wonder if it's because I also ended up with another question -do you think touch-action should go on the slider handle, also? What about the |
||
|
||
&::before { | ||
content: ""; | ||
display: block; | ||
|
Large diffs are not rendered by default.
Large diffs are not rendered by default.
Large diffs are not rendered by default.
Large diffs are not rendered by default.
Large diffs are not rendered by default.
Large diffs are not rendered by default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Color wheel should be added to this list, too!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@marissahuysentruyt already had it before these additions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like the color wheel index.css file was changed, so it should be added to this list of components, right? So that it gets the proper minor version bump as well?