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(Android): formSheet flex 1 support #2462

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

alduzy
Copy link
Member

@alduzy alduzy commented Oct 29, 2024

This PR was originally posted in react-navigation: react-navigation/react-navigation#12196
Since #2433 the changes should be applied here.

This change relies on replacing ScreenStackContent in react-navigation with this ScreenStackItem component imported from react-native-screens. Until it's done this PR is a draft. Keep that in mind if you want to test this change.

Description

This PR intents to add flex: 1 support to android formSheets. Currently when you set flex: 1 style to a wrapper component (quite usual thing to do) inside a screen with presentation: 'formSheet' it doesn't show up on android. On iOS it works fine.

This adjustment prevents the android sheet from breaking when given the flex: 1 style and ensures that its behaviour aligns better with iOS.

While iOS is given sheet specific styles, Android receives null. Passing same styles to both platforms makes the flex: 1 work as expected. To ensure that the scrollview on Android knows it's height and scrolls properly, a maxHeight: '100%' style is added.

Additionally, on Android there's no header in formSheet presentation. Thus we want to explicitly set it to hidden, as it may influence the content's layout.

Changes

  • adjusted layout styles for formSheet presentation.
  • ensured header is hidden on android sheet
  • added Test2462.tsx repro

Screenshots / GIFs

Before

without backgroundColor with backgroundColor
Screenshot 2024-10-29 at 11 44 38 Screenshot 2024-10-29 at 11 44 47

After

without backgroundColor with backgroundColor
Screenshot 2024-10-29 at 11 47 03 Screenshot 2024-10-29 at 11 43 57

Test code and steps to reproduce

  • use Test2462.tsx repro

Checklist

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

@alduzy alduzy requested review from satya164 and kkafar October 29, 2024 10:42
@satya164
Copy link
Collaborator

This change relies on replacing ScreenStackContent in react-navigation with this ScreenStackItem component imported from react-native-screens. Until it's done this PR is a draft.

The bugfix needs React Navigation to change this import, but this bugfix doesn't depend on React Navigation doing it first. Even if you merge it first here and React Navigation bumps the version afterwards, the outcome is the same. So please go ahead with merging and releasing this first. That way I can bump the version in React Navigation and make releases once.

Copy link
Collaborator

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

I'd suggest moving/copying explanation from PR to code comments - that way when someone is reading the code it'll be more obvious what's happening without having to visit PRs.

@alduzy alduzy marked this pull request as ready for review October 29, 2024 11:39
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.

I won't approve this until we understand why this works. I don't get why setting maxHeight this way would impact the layout as it did.

If you're able - please provide us with some insights - otherwise I'll be able to dive into it ~todays afternoon tomorrow morning

@alduzy
Copy link
Member Author

alduzy commented Oct 30, 2024

@kkafar I added few more lines to description to make it more clear.

Basically changing the styles on android from null to those used on iOS is the most important step that adds flex: 1 support.

Then, as I was testing the changes with various sheet contents, I had to add maxHeight and ensure the header is hidden on Android for the scrollview to layout and scroll correctly.

@alduzy alduzy requested review from satya164 and kkafar October 30, 2024 16:06
@RodolfoSilva
Copy link

@alduzy do you have any workaround suggestion for this?

@alduzy
Copy link
Member Author

alduzy commented Dec 16, 2024

@RodolfoSilva Don't use flex: 1 in sheet's screen container view and it should be all good.

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.

4 participants