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(iOS): header subviews layout on tab change #2385

Merged
merged 7 commits into from
Oct 7, 2024

Conversation

alduzy
Copy link
Member

@alduzy alduzy commented Oct 4, 2024

Description

This PR intents to fix header subviews incorrect layout when changing tabs.

The previous solution did layout the subviews correctly in the test cases, but triggered an undesirable layoutIfNeeded when going back from tab to tab. In such case the navigation layout happened without updating subview's layout metrics.

Moving the logic to subview resolves the issue as the re-layout is now triggered only when subview's layout metrics are updated.

Related fixes from the past: #2316, #2248

Changes

  • combined Test2231.tsx with Test432.tsx to create comprehensive test case
  • moved re-layout logic to subview

Screenshots / GIFs

Before

Screenshot 2024-10-04 at 10 10 15

After

Screenshot 2024-10-04 at 10 09 37

Test code and steps to reproduce

  • Use Test432.tsx repro

Checklist

  • Included code example that can be used to test this change
  • Ensured that CI passes

@alduzy alduzy requested a review from kkafar October 4, 2024 08:16
@alduzy alduzy marked this pull request as ready for review October 4, 2024 08:16
@alduzy alduzy marked this pull request as draft October 4, 2024 08:29
@alduzy alduzy marked this pull request as ready for review October 7, 2024 09:07
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.

Great job overall, few remarks below

ios/RNSScreenStackHeaderSubview.mm Outdated Show resolved Hide resolved
ios/RNSScreenStackHeaderSubview.mm Outdated Show resolved Hide resolved
ios/RNSScreenStackHeaderSubview.mm Outdated Show resolved Hide resolved
@alduzy alduzy requested a review from kkafar October 7, 2024 10:03
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.

Few more notes.

ios/RNSScreenStackHeaderSubview.mm Outdated Show resolved Hide resolved
@alduzy alduzy force-pushed the @alduzy/header-subviews-ios-fix branch from baec4e1 to 0667d88 Compare October 7, 2024 11:02
@alduzy alduzy force-pushed the @alduzy/header-subviews-ios-fix branch from 0667d88 to ea77267 Compare October 7, 2024 11:06
@alduzy alduzy requested a review from kkafar October 7, 2024 11:06
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.

Looks good, great job 🎉

@alduzy alduzy merged commit 91d89c4 into main Oct 7, 2024
5 checks passed
@alduzy alduzy deleted the @alduzy/header-subviews-ios-fix branch October 7, 2024 20:23
ja1ns pushed a commit to WiseOwlTech/react-native-screens that referenced this pull request Oct 9, 2024
## Description

This PR intents to fix header subviews incorrect layout when changing
tabs.

The previous solution did layout the subviews correctly in the test
cases, but triggered an undesirable `layoutIfNeeded` when going back
from tab to tab. In such case the navigation layout happened without
updating subview's layout metrics.

Moving the logic to subview resolves the issue as the re-layout is now
triggered only when subview's layout metrics are updated.

Related fixes from the past:
software-mansion#2316,
software-mansion#2248

## Changes

- combined `Test2231.tsx` with `Test432.tsx` to create comprehensive
test case
- moved re-layout logic to subview

## Screenshots / GIFs

### Before
![Screenshot 2024-10-04 at 10 10
15](https://github.com/user-attachments/assets/1a8a747e-fe1d-4b03-ba63-1891732d7d77)

### After
![Screenshot 2024-10-04 at 10 09
37](https://github.com/user-attachments/assets/68b3554f-d67d-47f4-946d-ace60e1bbf83)


## Test code and steps to reproduce

- Use `Test432.tsx` repro

## Checklist

- [x] Included code example that can be used to test this change
- [x] Ensured that CI passes
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.

2 participants