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: buggy behaviour of search bar / large title on Fabric with native stack v6 #11502

Merged

Conversation

kkafar
Copy link
Contributor

@kkafar kkafar commented Jul 26, 2023

Motivation

UIKit requires ScrollView to be at index 0 in given view's subview array to enable it's interaction with navigation bar. On Fabric DebugContainer view got flattened, however it was not removed from hierarchy -- instead the view was attached as first child of the RNSScreenView disabling system interaction between navigation bar and a scroll view.

See

for broader context and explanation.

Test plan

Before

Simulator.Screen.Recording.-.iPhone.14.-.2023-07-18.at.23.31.05.mp4

After

Simulator.Screen.Recording.-.iPhone.14.-.2023-07-18.at.23.28.42.mp4

@github-actions
Copy link

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

@codecov-commenter
Copy link

codecov-commenter commented Jul 26, 2023

Codecov Report

Patch coverage: 37.50% and project coverage change: -0.02% ⚠️

Comparison is base (cf836cb) 74.04% compared to head (8d4cb30) 74.02%.
Report is 2 commits behind head on 6.x.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Additional details and impacted files
@@            Coverage Diff             @@
##              6.x   #11502      +/-   ##
==========================================
- Coverage   74.04%   74.02%   -0.02%     
==========================================
  Files         177      177              
  Lines        5628     5628              
  Branches     2214     2213       -1     
==========================================
- Hits         4167     4166       -1     
- Misses       1412     1413       +1     
  Partials       49       49              
Files Changed Coverage Δ
packages/drawer/src/views/DrawerView.tsx 72.04% <0.00%> (ø)
.../native-stack/src/views/NativeStackView.native.tsx 78.94% <42.85%> (ø)
...s/native-stack/src/views/DebugContainer.native.tsx 87.50% <75.00%> (-12.50%) ⬇️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@kkafar
Copy link
Contributor Author

kkafar commented Jul 27, 2023

Asking for a review @kacperkapusciak

Copy link
Contributor Author

@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.

Just a remark for future reviewer

packages/native-stack/src/views/DebugContainer.native.tsx Outdated Show resolved Hide resolved
@kacperkapusciak kacperkapusciak self-requested a review July 28, 2023 06:37
@satya164 satya164 force-pushed the @kkafar/fix-searchbar-fabric-v6 branch from bf86c00 to e86baaa Compare July 31, 2023 12:15
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.

Thanks for the PR

@satya164 satya164 enabled auto-merge (squash) July 31, 2023 12:16
@satya164 satya164 merged commit cbdcbfb into react-navigation:6.x Aug 1, 2023
8 checks passed
@Gregoirevda
Copy link

@satya164 @kkafar This PR which is the only code diff in @react-navigation/native-stack 6.9.14 totally breaks the android search when not used with Fabric. 6.9.13 works fine. The search icon doesn't appear in the header. iOS seems to be working fine.

@github-actions
Copy link

github-actions bot commented Oct 3, 2023

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.

@kkafar
Copy link
Contributor Author

kkafar commented Oct 3, 2023

@Gregoirevda, would you mind posting a snack / repo so that I might reproduce? I've got working Android searchbar in my example app.

Ideally create a new issue.

@kkafar
Copy link
Contributor Author

kkafar commented Oct 4, 2023

@Gregoirevda is is also likely that the issue has been already fixed with software-mansion/react-native-screens#1883

New screens version will be released with this fix in couple of days.

@Gregoirevda
Copy link

@kkafar Great work, will give an update when available

@SamPoate
Copy link

FYI, this PR broke header Left/Right components that used absolute placed buttons on iOS. I've traced it back to the latest working package being "@react-navigation/native-stack": "6.9.13".

satya164 added a commit that referenced this pull request Nov 3, 2023
Header customization in native stack got broken in #11502. This change fixes it.
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

6 participants