-
Notifications
You must be signed in to change notification settings - Fork 672
added draggable property to ScrollView, closes #2260 #8512
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: master
Are you sure you want to change the base?
Conversation
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.
Thanks a lot for the PR!
This is a nice improvement and the code looks good.
I’m not sure if draggable
is the best name. Maybe something like mouse-drag
or enable-mouse-pan
would be clearer. The reason is that in the future, we want ScrollView
to always support dragging with a finger (touch), even if this property is false. But to do that, we first need support for detecting if the input is from a mouse or a finger (see #4352).
Right now, this property is only about dragging with the mouse, so the name should make that clear.
Also, we should keep the default behavior in the Material style, to dragging is enabled by default on android. It would be good to mention in the docs that the default can depend on the style.
What do you think?
@@ -117,6 +118,7 @@ export component ScrollView { | |||
flickable := Flickable { | |||
x: 0; | |||
y: 0; | |||
interactive: false; |
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.
This defaults to false because on android we should default to draggable.
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.
I'm a bit confused, wouldn't I want interactive: true;
to default to draggable? And, does this also apply for Cupertino?
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.
right sorry my comment was not clear.
It currently defaults to true for the material style (as opposed to all the other style which explicitly set interactive: false
The material style is different because this is a style which is primarily interacted with touch because of android.
While the other style are to be used on desktop.
It is true that cupertino is currently the default on iOS, but our iOS support is not finished yet.
I've renamed the property to |
Adds a
draggable
property to ScrollView that binds to theinteractive
property of the internal Flickable. Should close #2260.