[iOS] Fabric: Fixes Modal present splash when animation type is none#41853
[iOS] Fabric: Fixes Modal present splash when animation type is none#41853zhongwuzw wants to merge 4 commits into
Conversation
Base commit: 12b64b7 |
|
@dmytrorykun has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
|
@dmytrorykun Hi, friendly ping, any progress on reviewing this PR? |
cipolleschi
left a comment
There was a problem hiding this comment.
Hi @zhongwuzw, sorry for the very late reply. I left a review internally back in december but haven't realized that this was a PR from OSS. 🤦
Unfortunately, I don't think that the PR is good as it is. I left some suggestions to start with, but I want to explore the problem a little more to find a better solution that won't require weird path and dependencies.
There was a problem hiding this comment.
I wonder if we can find a way to avoid this.
There was a problem hiding this comment.
@cipolleschi Emm, perhaps we should not share a common ModalHostViewState and instead have separate ModalHostViewStates for different platforms?
There was a problem hiding this comment.
what if we pass a Size parameter which is {0,0} by default and then we look if there is a way to inject the size from the iOS mounting layer? 🤔
There was a problem hiding this comment.
@cipolleschi I tried to change the size before the mount view, but it's too late because we need to get the correct layout info in the layout phase
201d0a4 to
5a739c5
Compare
5a739c5 to
a6e5702
Compare
|
Sorry if this is taking too long. It is in my queue and I'll get to it at some point, hopefully soon! |
…/fix_modal_splash
|
Merge main branch to fix the conflict. |
…/fix_modal_splash
|
@cipolleschi Hi, any chance to keep forward this PR? :) |
|
Let me import this and let's see if some colleague has some ideas on how to approach this better or if they are happy with this! :D |
|
@cipolleschi merged this pull request in 14de1b7. |
|
This pull request was successfully merged by @zhongwuzw in 14de1b7 When will my fix make it into a release? | How to file a pick request? |
Summary:
Because the modal layout is triggered by state, it's an async operation after the first mount, so we can see the splash when present modal. now we can pass the screen size to the initial state which can layout in the first mount operation.
before:
https://github.com/facebook/react-native/assets/5061845/a39d519e-e2f6-42f1-8319-6216c88e9cf3
After:
https://github.com/facebook/react-native/assets/5061845/c7d59820-399b-4ea2-943d-d889971ea7ee
Changelog:
[IOS] [FIXED] - Fabric: Fixes Modal present splash when animation type is none
Test Plan:
RNTester Modal example, present modal in none animation mode.