-
-
Notifications
You must be signed in to change notification settings - Fork 807
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
feat: add enabled prop to draggable view #2160
base: master
Are you sure you want to change the base?
feat: add enabled prop to draggable view #2160
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 for submitting this PR, please address the comment i left
key="BottomSheetRootDraggableView" | ||
style={contentContainerStyle} | ||
enabled={enableContentPanningGesture} |
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 dont think we need to pass it as props, as it is already read internally from useBottomSheetInternal
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 is necessary so that BottomSheetDraggableView
can also be used when enableContentPanningGesture
of the BottomSheet
is set to false
, see PR description.
I've added an example DraggableViewExample
which showcases the usage.
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.
mmm i see, i think for scalability , i would move the enableContentPanningGesture
to each scrollable view instead. but we could go ahead with this temporary solution
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.
Yeah, in my case I need to wrap an external component so the solution of this MR would work better for me...
Motivation
The PR adds a simple
enabled
flag toBottomSheetDraggableView
which sets the enabled flag on the gesture.Previously
enableContentPanningGesture
was accessed viauseBottomSheetInternal
.By allowing to set the
enabled
flag on the component directly it is now possible to useBottomSheetDraggableView
even if theenableContentPanningGesture
of theBottomSheet
is set tofalse
and soBottomSheetDraggableView
can be used to only make part of the sheet's content draggable.Another side effect is the removal of:
which previously caused a remount of the content when
enableContentPanningGesture
was changed.