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

feat: add freezeOnBlur prop to enable/disable freeze per navigator & per screen #1538

Merged
merged 7 commits into from
Aug 16, 2022

Conversation

janicduplessis
Copy link
Contributor

@janicduplessis janicduplessis commented Jul 19, 2022

Description

We have a use case where we'd like to disable freeze for a particular screen. This adds a prop to allow setting if freeze is enabled on a per screen basis.

Changes

Added a freezeOnBlur prop to the Screen component.

Test code and steps to reproduce

Tested in an app that it is now possible to disable freeze for a particular screen and that previous behavior is kept if no prop is passed.

In repo: TestsExample/src/TestFreeze.tsx

Checklist

@kacperkapusciak kacperkapusciak self-requested a review July 22, 2022 09:03
@kacperkapusciak kacperkapusciak self-assigned this Jul 25, 2022
@kacperkapusciak kacperkapusciak requested review from WoLewicki and kkafar and removed request for kacperkapusciak July 25, 2022 15:00
@kacperkapusciak kacperkapusciak changed the title feat: allow setting if freeze is enabled per screen feat: add freezePreviousScreen and freezeInactiveScreens to disable freeze per navigator & per screen Jul 25, 2022
@kacperkapusciak
Copy link
Member

Thanks @janicduplessis! 🏅

I've renamed the props and added a way to disable freeze per navigator.

Fixes software-mansion/react-freeze#23

@kacperkapusciak
Copy link
Member

After a discussion with @WoLewicki and @satya164 I've decided to drop the option on the navigator and unify the logic under one freezeOnBlur prop that can disable and enable freezing per screen and/or per navigator with screenOptions.

Gonna add this to all navigators in @react-navigation soon™

@kacperkapusciak kacperkapusciak changed the title feat: add freezePreviousScreen and freezeInactiveScreens to disable freeze per navigator & per screen feat: add freezeOnBlur prop to enable/disable freeze per navigator & per screen Jul 26, 2022
@janicduplessis
Copy link
Contributor Author

@kacperkapusciak Thanks, sounds good. I was also planning to add it to @react-navigation, I should be able to do it in the next few days.

@kacperkapusciak
Copy link
Member

After back and forth discussions we've decided to remove the MaybeFreeze component. But why?

With the introduction of this PR and the implementation of freezeOnBlur prop developers can enable/disable freezing anytime in runtime. With this power React in the change of enabled in MaybeFreeze would have to unmount and remount the whole react tree including losing react all state etc.

  if (enabled) {
    return <DelayedFreeze freeze={freeze}>{children}</DelayedFreeze>;
  } else {
    return <>{children}</>;
  }

On the other hand, a bare Freeze component coming from react-freeze lib has no real side effects besides wrapping a component with a React.Suspense. In this code when Suspender is provided with false (the Freeze component) it defines a ref and it does nothing with it.

Remounting the whole React tree is way worse than wrapping every screen with an (almost) empty Suspense.

@kkafar kkafar linked an issue Aug 13, 2022 that may be closed by this pull request
Copy link
Member

@kkafar kkafar left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@kkafar kkafar merged commit 7653d3d into software-mansion:main Aug 16, 2022
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.

Can we exclude components on a screen from becoming frozen?
4 participants