-
-
Notifications
You must be signed in to change notification settings - Fork 785
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
Fix BottomSheetBackdrop "falsey" defaults #793
Fix BottomSheetBackdrop "falsey" defaults #793
Conversation
This PR is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 10 days. |
@gorhom, let me know if there's anything I can do to help get this PR approved. I'll continue to rebase as necessary since the changes are scoped to mostly the Backdrop component |
Coercion of 0 and false result in defaults being accidentally assigned to the BottomSheetBackdrop's props. This fix uses the null coalesce operator ?? to use either a user supplied value or fall back to the built-in default. Resolves gorhom#779
Fixed errors found by running `yarn typescript` and `yarn lint` to fix errors that could not be automatically fixed.
439b4d4
to
73e8cb2
Compare
This PR is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 10 days. |
Rebase verification (and un-stale): |
Hi, @gorhom Do you have any plan on this PR? |
This PR is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 10 days. |
@gorhom (ping) The fix is 3 Stalebots old. Would love to land this. |
hi @jakobo, thanks for submitting this PR, i left one comment regarding the name convention for the props. |
…SheetBackdrop_defaults * upstream/master: chore: updated react native portal library fix: always update container height to avoid races. (gorhom#919)(by @elan)
Applied the requested changes, @gorhom. Let me know if there's any other changes. 🎉 |
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 @jakobo for updating the naming! this will be release on the next patch release
This is a change to how
BottomSheetBackdrop
handles its falsey defaults. Values such as0
andfalse
will automatically assume their fallback values instead of allowing their literal0
orfalse
to be applied. There is a reproducible Snak in #779 which shows the problem.https://snack.expo.dev/@jakobo/bottomsheetbackdrop---appearsonindex-bug
Motivation
The fix is to ensure that the provided value was truly undefined before assigning the default. This patch uses nullish coalesce
??
to achieve that. The following code will never render a backdrop.Under the hood,
BottomSheetBackdrop
will assign1
forappearsOnIndex
because JS treats0
as false-like.Contributing Guidelines ✅