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

add check with navigation when PermissionAudio BottomSheet is open, a… #384

Closed
wants to merge 5 commits into from

Conversation

bohdanprog
Copy link
Collaborator

  • add check with navigation when PermissionAudio BottomSheet is open.
  • comment line with navigation function because those screens don't exist in the main branch.

@bohdanprog bohdanprog requested a review from EvanHahn May 28, 2024 11:11
@bohdanprog bohdanprog self-assigned this May 28, 2024
@bohdanprog bohdanprog added bug Something isn't working swmansion dev experience labels May 28, 2024
@ErikSin ErikSin removed the request for review from EvanHahn May 28, 2024 14:33
Copy link
Contributor

@ErikSin ErikSin left a comment

Choose a reason for hiding this comment

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

@@ -69,7 +69,8 @@ export const ThumbnailAndActionTab: FC<ThumbnailAndActionTab> = ({

const handleAudioPress = useCallback(() => {
Copy link
Contributor

Choose a reason for hiding this comment

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

navigation can be taken out of the dependency array for use callback

@bohdanprog
Copy link
Collaborator Author

@ErikSin here is the reason why we made that in that way. I agreed with that solution; unfortunately, I didn't check all the cases.
#375 (comment)

Here is solution where we handled all cases. We can change that if it looks good for you. Thanks

@bohdanprog bohdanprog requested a review from ErikSin May 28, 2024 19:56
@ErikSin
Copy link
Contributor

ErikSin commented May 28, 2024

Sorry im a little confused.

Right now, its just not working. When I give permissions for audio it is still asking me for permission.

@bohdanprog
Copy link
Collaborator Author

bohdanprog commented May 29, 2024

@ErikSin
I restored the old functionality and recorded the short video of how it works now.
https://github.com/digidem/comapeo-mobile/assets/69891500/9d1e0bf2-8e38-4aec-b583-1d5adde917ff

Copy link
Contributor

@ErikSin ErikSin left a comment

Choose a reason for hiding this comment

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

I think this flow is a little confusing.

When the user sets the permission in the settings, we should close the modal. This can be done in the listener:

useEffect(() => {
    const subscription = AppState.addEventListener('focus', () =>
      Audio.getPermissionsAsync().then(perm=>{
      setPermissionData(perm)
      if(perm.isGranted) closeModal()
      }),
    );
    return () => subscription.remove();
  }, []);

The problem with this, is the hook usePermissions in ThumbnailAndActionTab doesn't get updates. If we do what I suggested above: The user has set permission in the setting, permission is granted and the modal is closed automatically. But inThumbnailAndActionTab permissionResponse === false (because the hook doesn't track when permission are asked outside the app). Since permissionResponse === false, the user will press the audio button, and the modal will open again even though they have the permission.

What needs to happen is the state needs to be tracked in the parent component with one hook const [permissionData, setPermissionData] = useState<PermissionResponse>();

Right now you are using 2 hooks: the usePermission in the parent and useState<PermissionResponse>() in the modal; Intead use useState<PermissionResponse>() in the parent, and pass it values as props to the modal. And then the permissionData will be synced between the parent and the modal, meaning that the modal will close when the setting have been changed AND the parent will know that, and not attempt to open the modal again

@bohdanprog
Copy link
Collaborator Author

I think this flow is a little confusing.

When the user sets the permission in the settings, we should close the modal. This can be done in the listener:

useEffect(() => {
    const subscription = AppState.addEventListener('focus', () =>
      Audio.getPermissionsAsync().then(perm=>{
      setPermissionData(perm)
      if(perm.isGranted) closeModal()
      }),
    );
    return () => subscription.remove();
  }, []);

The problem with this, is the hook usePermissions in ThumbnailAndActionTab doesn't get updates. If we do what I suggested above: The user has set permission in the setting, permission is granted and the modal is closed automatically. But inThumbnailAndActionTab permissionResponse === false (because the hook doesn't track when permission are asked outside the app). Since permissionResponse === false, the user will press the audio button, and the modal will open again even though they have the permission.

What needs to happen is the state needs to be tracked in the parent component with one hook const [permissionData, setPermissionData] = useState<PermissionResponse>();

Right now you are using 2 hooks: the usePermission in the parent and useState<PermissionResponse>() in the modal; Intead use useState<PermissionResponse>() in the parent, and pass it values as props to the modal. And then the permissionData will be synced between the parent and the modal, meaning that the modal will close when the setting have been changed AND the parent will know that, and not attempt to open the modal again

Okay, I thought we shouldn't have to close the modal after the user has set permission in the setting and permission is granted I thought we should stay in the same place and after he presses the Allow button we navigate him to another screen. Now it makes more sens to me thanks for your suggestion. I'm going to change that.

@ErikSin
Copy link
Contributor

ErikSin commented May 29, 2024

Okay, I thought we shouldn't have to close the modal after the user has set permission in the setting and permission is granted I thought we should stay in the same place and after he presses the Allow button we navigate him to another screen. Now it makes more sens to me thanks for your suggestion. I'm going to change that.

Just for clarity, what should happen is that when they change their permissions in the setting, the modal should close and the user should be navigated to the audio page. Obviously that is out of scope for this PR, so we should just close the modal for this PR

@bohdanprog bohdanprog requested a review from ErikSin May 30, 2024 21:34
@ErikSin ErikSin assigned achou11 and unassigned bohdanprog Jun 18, 2024
@ErikSin ErikSin closed this Sep 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working dev experience swmansion
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants