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

fix: Check cardStyleInterpolator name to allow for custom animation as well #11209

Merged
merged 1 commit into from Feb 13, 2023

Conversation

drager
Copy link
Contributor

@drager drager commented Feb 10, 2023

We got a custom cardStyleInterpolator in our app that actually calls forModalPresentationIOS but we add some own customizations to it. This worked fine in v5 of react-navigation, and started to act weird after the upgrade to v6, the screen under the modal was displayed right until the animation was finished, then the screen under disappeared, which is unintended.

This PR just checks for the name instead. Making it do the same as this check: https://github.com/react-navigation/react-navigation/blob/main/packages/stack/src/views/Stack/Card.tsx#L580 and will allow for custom cardStyleInterpolators.

This can be reproduced by creating your own cardStyleIntepolator and then call forModalPresentationIOS:

import type {StackCardInterpolationProps} from "@react-navigation/stack";
import {CardStyleInterpolators} from "@react-navigation/stack";

export const forModalPresentationIOS = ({
    index,
    current,
    next,
    inverted,
    layouts,
    insets,
    ...rest
}: StackCardInterpolationProps) => {
    const defaultAnimations = CardStyleInterpolators.forModalPresentationIOS({
        index,
        current,
        next,
        inverted,
        layouts,
        insets,
        ...rest,
    });
    return defaultAnimations
};


const ModalStack = createStackNavigator<ModalStackParamList>();

export function ModalNavigator() {
    return (
        <ModalStack.Navigator>
            <ModalStack.Screen
                name='Modal'
                component={() => <View />}
                options={{
                    gestureEnabled: true,
                    gestureDirection: "vertical",
                    cardOverlayEnabled: true,
                    cardStyleInterpolator: forModalPresentationIOS
               }}
            />
        </ModalStack.Navigator>
}

@github-actions
Copy link

Hey drager! Thanks for opening your first pull request in this repo. If you haven't already, make sure to read our contribution guidelines.

@netlify
Copy link

netlify bot commented Feb 10, 2023

Deploy Preview for react-navigation-example ready!

Name Link
🔨 Latest commit 765564a
🔍 Latest deploy log https://app.netlify.com/sites/react-navigation-example/deploys/63e9d7d40898b90008451722
😎 Deploy Preview https://deploy-preview-11209--react-navigation-example.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

Copy link
Member

@satya164 satya164 left a comment

Choose a reason for hiding this comment

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

Thank you for the PR

@satya164 satya164 enabled auto-merge (squash) February 12, 2023 18:54
@codecov-commenter
Copy link

codecov-commenter commented Feb 12, 2023

Codecov Report

Base: 74.09% // Head: 74.09% // No change to project coverage 👍

Coverage data is based on head (b0c37dc) compared to base (061fb13).
Patch coverage: 0.00% of modified lines in pull request are covered.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

Additional details and impacted files
@@           Coverage Diff           @@
##             main   #11209   +/-   ##
=======================================
  Coverage   74.09%   74.09%           
=======================================
  Files         176      176           
  Lines        5598     5598           
  Branches     2193     2193           
=======================================
  Hits         4148     4148           
  Misses       1401     1401           
  Partials       49       49           
Impacted Files Coverage Δ
packages/stack/src/views/Stack/CardStack.tsx 84.10% <0.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@satya164
Copy link
Member

satya164 commented Feb 12, 2023

Please fix the lint errors so that this can be merged

auto-merge was automatically disabled February 13, 2023 06:20

Head branch was pushed to by a user without write access

@github-actions
Copy link

Hey @drager! Thanks for opening your first pull request in this repo. If you haven't already, make sure to read our contribution guidelines.

@drager
Copy link
Contributor Author

drager commented Feb 13, 2023

Please fix the lint errors so that this can be merged

Done!

@satya164 satya164 enabled auto-merge (squash) February 13, 2023 07:49
@satya164 satya164 merged commit a5050ba into react-navigation:main Feb 13, 2023
satya164 pushed a commit that referenced this pull request Feb 13, 2023
…s well (#11209)

We got a custom `cardStyleInterpolator` in our app that actually calls
[forModalPresentationIOS](https://github.com/react-navigation/react-navigation/blob/main/packages/stack/src/TransitionConfigs/CardStyleInterpolators.tsx#L93)
but we add some own customizations to it. This worked fine in `v5` of
react-navigation, and started to act weird after the upgrade to `v6`,
the screen under the modal was displayed right until the animation was
finished, then the screen under disappeared, which is unintended.

This PR just checks for the name instead. Making it do the same as this
check:
https://github.com/react-navigation/react-navigation/blob/main/packages/stack/src/views/Stack/Card.tsx#L580
and will allow for custom cardStyleInterpolators.

This can be reproduced by creating your own cardStyleIntepolator and
then call `forModalPresentationIOS`:

```ts
import type {StackCardInterpolationProps} from "@react-navigation/stack";
import {CardStyleInterpolators} from "@react-navigation/stack";

export const forModalPresentationIOS = ({
    index,
    current,
    next,
    inverted,
    layouts,
    insets,
    ...rest
}: StackCardInterpolationProps) => {
    const defaultAnimations = CardStyleInterpolators.forModalPresentationIOS({
        index,
        current,
        next,
        inverted,
        layouts,
        insets,
        ...rest,
    });
    return defaultAnimations
};


const ModalStack = createStackNavigator<ModalStackParamList>();

export function ModalNavigator() {
    return (
        <ModalStack.Navigator>
            <ModalStack.Screen
                name='Modal'
                component={() => <View />}
                options={{
                    gestureEnabled: true,
                    gestureDirection: "vertical",
                    cardOverlayEnabled: true,
                    cardStyleInterpolator: forModalPresentationIOS
               }}
            />
        </ModalStack.Navigator>
}
```
@cixio
Copy link

cixio commented Feb 15, 2023

Hi, I think with this change there is a new bug on iOS (only on production build):

stack@6.3.12
IMG_1835

stack@6.3.13
IMG_1834

The gray background comes to view when the animation is complete.

Should I open a new issue?

@github-actions
Copy link

Hey! This issue is closed and isn't watched by the core team. You are welcome to discuss the issue with others in this thread, but if you think this issue is still valid and needs to be tracked, please open a new issue with a repro.

@satya164
Copy link
Member

@cixio feel free to send a PR. the function name is likey minified in production and causing this. so the fix would be to do something like this:

i !==
  findLastIndex((scenes, scene) => {
    const { cardStyleInterpolator } = scene.descriptor.options;

    return (
      cardStyleInterpolator === forModalPresentationIOS ||
      cardStyleInterpolator?.name === "forModalPresentationIOS"
    );
  });

The implementation of findLastIndex could be:

const findLastIndex = (array, callback) => {
  for (var i = array.length - 1; i >= 0; i--) {
    if (callback(array[i])) {
      return i;
    }
  }

  return -1;
};

@cixio
Copy link

cixio commented Feb 15, 2023

I'd love to do a PR, but I honestly have no idea what's going on here. I could only determine that the bug must be due to this change. Sorry :(

@satya164
Copy link
Member

@cixio please try @react-navigation/stack@6.3.14

@cixio
Copy link

cixio commented Feb 16, 2023

works perfekt. thanks! 🥇

satya164 pushed a commit that referenced this pull request Feb 17, 2023
…s well (#11209)

We got a custom `cardStyleInterpolator` in our app that actually calls
[forModalPresentationIOS](https://github.com/react-navigation/react-navigation/blob/main/packages/stack/src/TransitionConfigs/CardStyleInterpolators.tsx#L93)
but we add some own customizations to it. This worked fine in `v5` of
react-navigation, and started to act weird after the upgrade to `v6`,
the screen under the modal was displayed right until the animation was
finished, then the screen under disappeared, which is unintended.

This PR just checks for the name instead. Making it do the same as this
check:
https://github.com/react-navigation/react-navigation/blob/main/packages/stack/src/views/Stack/Card.tsx#L580
and will allow for custom cardStyleInterpolators.

This can be reproduced by creating your own cardStyleIntepolator and
then call `forModalPresentationIOS`:

```ts
import type {StackCardInterpolationProps} from "@react-navigation/stack";
import {CardStyleInterpolators} from "@react-navigation/stack";

export const forModalPresentationIOS = ({
    index,
    current,
    next,
    inverted,
    layouts,
    insets,
    ...rest
}: StackCardInterpolationProps) => {
    const defaultAnimations = CardStyleInterpolators.forModalPresentationIOS({
        index,
        current,
        next,
        inverted,
        layouts,
        insets,
        ...rest,
    });
    return defaultAnimations
};


const ModalStack = createStackNavigator<ModalStackParamList>();

export function ModalNavigator() {
    return (
        <ModalStack.Navigator>
            <ModalStack.Screen
                name='Modal'
                component={() => <View />}
                options={{
                    gestureEnabled: true,
                    gestureDirection: "vertical",
                    cardOverlayEnabled: true,
                    cardStyleInterpolator: forModalPresentationIOS
               }}
            />
        </ModalStack.Navigator>
}
```
satya164 pushed a commit that referenced this pull request Feb 17, 2023
…s well (#11209)

We got a custom `cardStyleInterpolator` in our app that actually calls
[forModalPresentationIOS](https://github.com/react-navigation/react-navigation/blob/main/packages/stack/src/TransitionConfigs/CardStyleInterpolators.tsx#L93)
but we add some own customizations to it. This worked fine in `v5` of
react-navigation, and started to act weird after the upgrade to `v6`,
the screen under the modal was displayed right until the animation was
finished, then the screen under disappeared, which is unintended.

This PR just checks for the name instead. Making it do the same as this
check:
https://github.com/react-navigation/react-navigation/blob/main/packages/stack/src/views/Stack/Card.tsx#L580
and will allow for custom cardStyleInterpolators.

This can be reproduced by creating your own cardStyleIntepolator and
then call `forModalPresentationIOS`:

```ts
import type {StackCardInterpolationProps} from "@react-navigation/stack";
import {CardStyleInterpolators} from "@react-navigation/stack";

export const forModalPresentationIOS = ({
    index,
    current,
    next,
    inverted,
    layouts,
    insets,
    ...rest
}: StackCardInterpolationProps) => {
    const defaultAnimations = CardStyleInterpolators.forModalPresentationIOS({
        index,
        current,
        next,
        inverted,
        layouts,
        insets,
        ...rest,
    });
    return defaultAnimations
};


const ModalStack = createStackNavigator<ModalStackParamList>();

export function ModalNavigator() {
    return (
        <ModalStack.Navigator>
            <ModalStack.Screen
                name='Modal'
                component={() => <View />}
                options={{
                    gestureEnabled: true,
                    gestureDirection: "vertical",
                    cardOverlayEnabled: true,
                    cardStyleInterpolator: forModalPresentationIOS
               }}
            />
        </ModalStack.Navigator>
}
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants