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

Adding freezeOnBlur for android and iOS #113

Conversation

shubhamguptadream11
Copy link
Contributor

@shubhamguptadream11 shubhamguptadream11 commented Nov 3, 2024

Solves this: #71

Adding freezeOnBlur property.

@shubhamguptadream11 shubhamguptadream11 marked this pull request as ready for review November 3, 2024 12:27
@shubhamguptadream11
Copy link
Contributor Author

Hey @okwasniewski,

I've implemented the freezeOnBlur property, following the same approach as in the JS bottom tabs. I've tested it on Android, both with enableFreeze() globally and by overriding it through options for individual tabs. Currently, I’m testing this on iOS to ensure everything works smoothly there as well.

Meanwhile, feel free to review the PR when you have a chance.

Thanks!

@okwasniewski okwasniewski linked an issue Nov 3, 2024 that may be closed by this pull request
src/TabView.tsx Outdated
Comment on lines 263 to 267
<MaybeScreenContainer
enabled={detachInactiveScreens}
hasTwoStates
style={styles.container}
>
Copy link
Collaborator

@okwasniewski okwasniewski Nov 3, 2024

Choose a reason for hiding this comment

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

Have you tested it on both new and old architecture? I feel like this might break passing children on new arch as technically now we have only one child.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Till now I tested this on Android for both new and old arch.
I will be testing it on iOS today.

@okwasniewski
Copy link
Collaborator

I got instant crash on new arch when going into the example

CleanShot 2024-11-03 at 18 57 55@2x

@shubhamguptadream11
Copy link
Contributor Author

@okwasniewski Fixed the iOS crash and tested it on Old and New arch for both platforms. Seems to be working fine now with react-native-screens enabled and disabled.

Copy link
Collaborator

@okwasniewski okwasniewski 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 working on this!

Comment on lines 30 to 36
if (Platform.OS === 'android' && Screens?.screensEnabled()) {
return (
<Screens.ScreenContainer enabled={enabled} {...rest}>
{children}
</Screens.ScreenContainer>
);
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks like we don't have the navigation container on iOS (JS Tabs have it) but it works without it?

That's weird 😅

If the ScreenContainer is required then adding this might not be possible. As we need to have multiple children here

@tboba Could you help here? 🙏

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@okwasniewski Yeah on iOS it works even without Screen Container.

In case of Android we need ScreenContainer and I just checked that It worked even with hasTwoStates={false} on both platforms with New and Old Arch.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hey, @shubhamguptadream11 after consulting with react-native-screens team (thanks @kkafar, @tboba) it looks like detachInactiveScreens won't work without the ScreenContainer.

Can you change the PR to only add freezeOnBlur?

@okwasniewski
Copy link
Collaborator

Hey, sorry for the delay on review of this PR, I'm trying to get in touch with someone from react-native-screens team to check if we will be okay without the ScreenContainer wrapper on iOS.

@Nodonisko
Copy link
Contributor

Any updates on this PR? :)

@shubhamguptadream11
Copy link
Contributor Author

Hi @Nodonisko
Actually I am not available in past few weeks.
Currently I am working on benchmarking native bottom tabs with JS one. Will pick this PR after that.

Copy link

changeset-bot bot commented Dec 11, 2024

⚠️ No Changeset found

Latest commit: 4cbdd6d

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@okwasniewski
Copy link
Collaborator

Merged in #207 thanks @shubhamguptadream11

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

Successfully merging this pull request may close these issues.

Implement freezeOnBlur
3 participants