-
-
Notifications
You must be signed in to change notification settings - Fork 799
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 useBottomSheetInternal_unsafe & useBottomSheetModalInternal… #740
feat: add useBottomSheetInternal_unsafe & useBottomSheetModalInternal… #740
Conversation
…_unsafe to access context
…odalInternal_unsafe
I was looking at something similar for the exact same use case. Rather than adding a new hook, maybe we can use an argument to the current hook? It currently does not have any argument, so adding a new one should not cause any breakage. |
Hi @renchap, at first time I also tried to implement using one hook with an param. But I struggled with the types. Hopefully this new version is fitting better. But please evaluate the types. I have used |
It should be cleaner to use Function Overloads like described here: https://www.typescriptlang.org/docs/handbook/2/functions.html#function-overloads Something like: function useBottomSheetInternal(): BottomSheetInternalContextType
function useBottomSheetInternal(unsafe: true): BottomSheetInternalContextType
export function useBottomSheetInternal(unsafe: false): BottomSheetInternalContextType | null {
const context = useContext(BottomSheetInternalContext);
if (unsafe !== true && context === null) {
throw "'useBottomSheetInternal' cannot be used out of the BottomSheet!";
}
return context;
} I did not test it but it should be the way to do it properly. |
I have changed it to function overloading. should now be ready :) |
@jembach @renchap I think this PR solves problem only partially. I have very similar issue when I try to use
react-native-bottom-sheet/src/components/bottomSheetBackdrop/BottomSheetBackdrop.tsx Line 36 in d951a19
and I see this bug |
@likern I support your problem to be solved. But your problem is affecting much more code than i thought about and will definitely cause a breaking change accross the hole app. One solution could be to create a new global Context that provides the information about if it could be accessed unsafe. The other solution would be that everything could return null and it must be handled every time. At least i cannot really understand the use case when you render the backdrop without the BottomSheetModal. I would suggest to create another PR to solve your problem. |
@jembach My use-case is described in this issue #747. As soon as we use modal, we might have shared backdrop component, not tied to behaviour of one I just wanted to mention that this problem exists. I couldn't solve it easily, just will reimplement |
Hi @gorhom how does it look to merge this request? I'll require it to solve some issues in our app. |
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. |
Hi @gorhom, could you spent a look 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. |
This would still be very useful! |
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. |
Still useful! |
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. |
Still useful :) |
@jembach for submitting this pr! |
…_unsafe to access context
Please provide enough information so that others can review your pull request:
Motivation
By using this package I discovered the problem of integrating the bottom sheet in combination with our TextInput component. While trying to add your supported solution for handling keyboard to our custom TextInput we run into the problem, that our app every time crashes, when our TextInput isn't rendered into a bottom sheet.
Therefore I have added two additional hooks which aren't checking if the context is present or not. Therefore i have called them unsafe. But with this solution we can check self if the context is present or not and react to each case.
The reason why I haven't updated the existing hooks is on the one hand that mostly everything in this package is relying on these hooks and on the other hand this could be cause for some user a breaking change.