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

Adjust onSettle index of BottomSheet #946

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

sieu-db
Copy link
Collaborator

@sieu-db sieu-db commented Aug 28, 2024

Overview

  • The callback index returned already mapped to provided point, we don't need to manual map it

Demo

Screen.Recording.2024-08-28.at.18.08.39.mov

🚀 This description was created by Ellipsis for commit f115228

Summary:

Updated BottomSheet component to use the index directly in onChange and added logging in BottomSheetExample.

Key points:

  • Updated onChange in packages/core/src/components/BottomSheet/BottomSheet.tsx to use the index directly.
  • Removed manual mapping of index in onChange.
  • Added onSettle callback in example/src/BottomSheetExample.tsx to log the new index.

Generated with ❤️ by ellipsis.dev

Copy link

linear bot commented Aug 28, 2024

@sieu-db sieu-db changed the title Adjust onChange index of BottomSheet Adjust onSettle index of BottomSheet Aug 28, 2024
Copy link

Published version: @draftbit/[email protected]

@@ -109,7 +109,7 @@ const BottomSheet = React.forwardRef<BottomSheetComponent, BottomSheetProps>(
borderWidth,
borderColor: borderColor ?? theme.colors.border.brand,
}}
onChange={(index) => onSettle?.(mappedSnapPoints.length - index - 1)}
onChange={(index) => onSettle?.(index)}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think this is the issue. Looking at the linear issue (and testing on my own), seems the bottom sheet has 4 snap points, even though by default there should only be 3 (top, middle, bottom). I think this inconsistency is what is leading to the incorrect index, but the existing logic for this is still correct.

Copy link
Collaborator

@YoussefHenna YoussefHenna Aug 28, 2024

Choose a reason for hiding this comment

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

Seems there's always an additional snap point for some reason, hmm..

Copy link
Collaborator

Choose a reason for hiding this comment

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

I can investigate this later today, but let me know if you have any idea why this could be happening.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ya, I will investigate more today 🙏 thanks for point out

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants