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

feat: add enabled prop to draggable view #2160

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

geovie
Copy link

@geovie geovie commented Feb 13, 2025

Motivation

The PR adds a simple enabled flag to BottomSheetDraggableView which sets the enabled flag on the gesture.
Previously enableContentPanningGesture was accessed via useBottomSheetInternal.

By allowing to set the enabled flag on the component directly it is now possible to use BottomSheetDraggableView even if the enableContentPanningGesture of the BottomSheet is set to false and so BottomSheetDraggableView can be used to only make part of the sheet's content draggable.

Another side effect is the removal of:

const DraggableView = enableContentPanningGesture
     ? BottomSheetDraggableView
     : Animated.View;

which previously caused a remount of the content when enableContentPanningGesture was changed.

Copy link
Owner

@gorhom gorhom left a 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}
Copy link
Owner

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

Copy link
Author

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.

Copy link
Owner

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

Copy link
Author

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...

@gorhom gorhom self-assigned this Feb 13, 2025
@gorhom gorhom added the v5 label Feb 13, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants