feat(iOS): add shadow to custom push pop transitions #2239

maksg · 2024-07-08T13:54:31Z

Description

When using full screen swipe back there was no shadow under the view.
Related PR #2234 with discussion about this change.

Corresponding PR in react-navigation:

feat: add fullScreenSwipeShadowEnabled prop to NativeStackView react-navigation/react-navigation#12053

This PR add fullScreenSwipeShadowEnabled prop.

Changes

Added shadow to transition in animateSimplePushWithTransitionContext:toVC:fromVC:.

Test code and steps to reproduce

Run Test2227 in TestsExample app.
Push screen with Go to Details.
Swipe back with full screen swipe gesture.
Before this fix there would be no shadow during transition with full screen swipe back.

Checklist

Included code example that can be used to test this change
Updated TS types
Updated documentation:
Ensured that CI passes

WoLewicki

Left some comments.

WoLewicki · 2024-07-09T14:15:37Z

guides/GUIDE_FOR_LIBRARY_AUTHORS.md

+### `fullScreenSwipeShadowEnabled` (iOS only)
+
+Boolean indicating whether the full screen dismiss gesture has shadow under view during transition. The gesture uses custom transition and thus
+doesn't have a shadow by default. When enabled a custom shadow view is added during the transition which tries to mimick the


Suggested change

doesn't have a shadow by default. When enabled a custom shadow view is added during the transition which tries to mimick the

doesn't have a shadow by default. When enabled, a custom shadow view is added during the transition, which tries to mimick the

Same for other occourences of this comment.

WoLewicki · 2024-07-09T14:19:15Z

src/native-stack/views/NativeStackView.tsx

@@ -216,6 +217,9 @@ const RouteView = ({
    if (fullScreenSwipeEnabled === undefined) {
      fullScreenSwipeEnabled = true;
    }
+    if (fullScreenSwipeShadowEnabled === undefined) {
+      fullScreenSwipeShadowEnabled = true;


If you do it like this, isn't it true by default?

I was thinking the same but fullScreenSwipeEnabled does it this way and it also is false by default. I think it is set elsewhere as false so this doesn't matter. But I can change that and maybe fullScreenSwipeEnabled also.

But the false value must be set somewhere right? Better to make this explicit.

Maybe it would be better to remove this altogether? Now I see that this is only set when swipeDirection === 'vertical' so it doesn't make sense to keep it there if it's not set for other swipe directions. Unless I should move it out of this if.

I think this prop is not dependent on any other props so we can remove it?

kkafar

The things we lack here:

Update codegen interfaces for Android: run build on FabricExample & copy interfaces for Paper or wait for merge of this PR and then trigger build on Android / add a commit.
Add a stub implementation for Android as done here, for example.

kkafar · 2024-07-09T14:52:50Z

src/native-stack/views/NativeStackView.tsx

@@ -216,6 +217,9 @@ const RouteView = ({
    if (fullScreenSwipeEnabled === undefined) {
      fullScreenSwipeEnabled = true;
    }
+    if (fullScreenSwipeShadowEnabled === undefined) {
+      fullScreenSwipeShadowEnabled = true;


But the false value must be set somewhere right? Better to make this explicit.

kkafar

Other than the points I emphasise below I think we're good.

kkafar · 2024-07-12T07:28:05Z

src/native-stack/views/NativeStackView.tsx

@@ -202,6 +202,7 @@ const RouteView = ({
  let {
    customAnimationOnSwipe,
    fullScreenSwipeEnabled,
+    fullScreenSwipeShadowEnabled,


Suggested change

fullScreenSwipeShadowEnabled,

fullScreenSwipeShadowEnabled = false,

kkafar · 2024-07-12T07:28:18Z

src/native-stack/views/NativeStackView.tsx

+  if (fullScreenSwipeShadowEnabled === undefined) {
+    fullScreenSwipeShadowEnabled = false;
+  }


Suggested change

if (fullScreenSwipeShadowEnabled === undefined) {

fullScreenSwipeShadowEnabled = false;

}

kkafar · 2024-07-12T07:32:17Z

react-navigation

The problem with bumping react-navigation in such way is that it will be overridden by next PR that tries to do the same. We need a PR to react-navigation, merge it there to main & bump react-navigation here to new main revision.

This change should be reverted & we should wait for progress on react-navigation PR.

This is a PR to add a prop for enabling shadow during full screen swipe transition on iOS. Required for software-mansion/react-native-screens#2239.

kkafar · 2024-07-23T07:00:24Z

Hey, if we are to proceed this one, you need to bump react navigation to it's recent main & incorporate these changes here. Obviously also resolve the merge conflicts.

…cting options

kkafar

Okay, I think we're good here. Great job! 🎉

This is a PR to add a prop for enabling shadow during full screen swipe transition on iOS. Required for software-mansion/react-native-screens#2239.

…adow during swipe gesture (software-mansion#2239) ## Description When using full screen swipe back there was no shadow under the view. Related PR software-mansion#2234 with discussion about this change. Corresponding PR in `react-navigation`: * react-navigation/react-navigation#12053 ## Changes Added shadow to transition in animateSimplePushWithTransitionContext:toVC:fromVC:. ## Test code and steps to reproduce 1. Run `Test2227` in TestsExample app. 2. Push screen with Go to Details. 3. Swipe back with full screen swipe gesture. 5. Before this fix there would be no shadow during transition with full screen swipe back. ## Checklist - [x] Included code example that can be used to test this change - [ ] Updated TS types - [x] Updated documentation:  - [x] https://github.com/software-mansion/react-native-screens/blob/main/guides/GUIDE_FOR_LIBRARY_AUTHORS.md - [x] https://github.com/software-mansion/react-native-screens/blob/main/native-stack/README.md - [x] https://github.com/software-mansion/react-native-screens/blob/main/src/types.tsx - [x] https://github.com/software-mansion/react-native-screens/blob/main/src/native-stack/types.tsx - [ ] Ensured that CI passes

maksg requested review from tboba, WoLewicki and kkafar July 8, 2024 13:54

maksg mentioned this pull request Jul 8, 2024

fix(iOS): push and pop transitions change after full screen back swipe #2234

Merged

8 tasks

feat(iOS): add shadow to custom push pop transitions

5c864c4

maksg force-pushed the @maksg/shadow-during-custom-push-pop branch from 15a9ac9 to 5c864c4 Compare July 9, 2024 10:46

feat(iOS): add prop to enable full screen swipe shadow

138f6d7

maksg mentioned this pull request Jul 9, 2024

feat: add fullScreenSwipeShadowEnabled prop to NativeStackView react-navigation/react-navigation#12053

Merged

WoLewicki reviewed Jul 9, 2024

View reviewed changes

kkafar reviewed Jul 9, 2024

View reviewed changes

fix: transition shadow Android props and typos

b7055c5

kkafar reviewed Jul 12, 2024

View reviewed changes

maksg added 2 commits July 23, 2024 10:52

chore: set default fullScreenGestureShadowEnabled value during destru…

6229afb

…cting options

Merge branch 'main' into @maksg/shadow-during-custom-push-pop

577dfc0

maksg requested review from kkafar and WoLewicki July 23, 2024 11:11

kkafar approved these changes Jul 23, 2024

View reviewed changes

maksg merged commit f6bd9cf into main Jul 23, 2024
9 checks passed

maksg deleted the @maksg/shadow-during-custom-push-pop branch July 23, 2024 15:55

WesleyR10 mentioned this pull request Aug 21, 2024

[Snyk] Upgrade react-native-screens from 3.31.1 to 3.33.0 WesleyR10/IgniteTeams#5

Open

shonore mentioned this pull request Aug 22, 2024

[Snyk] Upgrade react-native-screens from 3.0.0 to 3.33.0 shonore/Fickle#1609

Open

2lambda123 mentioned this pull request Aug 22, 2024

[Snyk] Upgrade react-native-screens from 3.32.0 to 3.33.0 2lambda123/abp#163

Merged

Cabreira97 mentioned this pull request Aug 23, 2024

[Snyk] Upgrade react-native-screens from 3.31.1 to 3.33.0 Cabreira97/Connect-Tech#5

Closed

2lambda123 mentioned this pull request Aug 26, 2024

[Snyk] Upgrade react-native-screens from 3.33.0 to 3.34.0 2lambda123/abp#164

Merged

shonore mentioned this pull request Sep 3, 2024

[Snyk] Upgrade react-native-screens from 3.0.0 to 3.34.0 shonore/Fickle#1669

Open

undrthegraveyard mentioned this pull request Sep 4, 2024

[Snyk] Upgrade react-native-screens from 3.31.1 to 3.34.0 undrthegraveyard/gigslick#5

Closed

asraful-cse mentioned this pull request Sep 6, 2024

[Snyk] Upgrade react-native-screens from 3.11.1 to 3.34.0 asraful-cse/planet_app_react_native#412

Open

This was referenced Sep 22, 2024

[Snyk] Upgrade: , , , ramda, react-native, react-native-calendars, react-native-config, react-native-screens Olaw2jr/sushi#18

Open

[Snyk] Upgrade: , , , ramda, react-native, react-native-calendars, react-native-config, react-native-screens Olaw2jr/sushi#19

Open

Ayush-Bulbule mentioned this pull request Oct 2, 2024

[Snyk] Upgrade react-native-screens from 3.22.1 to 3.34.0 Ayush-Bulbule/FoodleApp#5

Merged

tobistudio mentioned this pull request Oct 3, 2024

[Snyk] Upgrade react-native-screens from 3.27.0 to 3.34.0 tobistudio/React_Native_Audio#75

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(iOS): add shadow to custom push pop transitions #2239

feat(iOS): add shadow to custom push pop transitions #2239

maksg commented Jul 8, 2024 •

edited by kkafar

Loading

WoLewicki left a comment

WoLewicki Jul 9, 2024

WoLewicki Jul 9, 2024

WoLewicki Jul 9, 2024

maksg Jul 9, 2024

kkafar Jul 9, 2024

maksg Jul 9, 2024

WoLewicki Jul 9, 2024

kkafar left a comment

kkafar Jul 9, 2024

kkafar left a comment

kkafar Jul 12, 2024

kkafar Jul 12, 2024

kkafar Jul 12, 2024

kkafar commented Jul 23, 2024

kkafar left a comment

	doesn't have a shadow by default. When enabled a custom shadow view is added during the transition which tries to mimick the
	doesn't have a shadow by default. When enabled, a custom shadow view is added during the transition, which tries to mimick the

	fullScreenSwipeShadowEnabled,
	fullScreenSwipeShadowEnabled = false,

	if (fullScreenSwipeShadowEnabled === undefined) {
	fullScreenSwipeShadowEnabled = false;
	}

feat(iOS): add shadow to custom push pop transitions #2239

feat(iOS): add shadow to custom push pop transitions #2239

Conversation

maksg commented Jul 8, 2024 • edited by kkafar Loading

Description

Changes

Test code and steps to reproduce

Checklist

WoLewicki left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kkafar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kkafar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kkafar commented Jul 23, 2024

kkafar left a comment

Choose a reason for hiding this comment

maksg commented Jul 8, 2024 •

edited by kkafar

Loading