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.

Sign up for GitHub

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

feat: proper snapshot when having multiple modals #2121

Merged
merged 3 commits into from
May 14, 2024
Merged

feat: proper snapshot when having multiple modals #2121

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ef608e8
feat: proper snapshot when having multiple modals
WoLewicki May 8, 2024
46e90b0
Merge branch 'main' into @wolewicki/proper-snapshot-in-modals
WoLewicki May 13, 2024
8211f25
fix: restore needed code
WoLewicki May 13, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion ios/RNSScreenStack.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1175,7 +1175,11 @@ - (void)unmountChildComponentView:(UIView<RCTComponentViewProtocol> *)childCompo

- (void)takeSnapshot
{
_snapshot = [_controller.visibleViewController.view snapshotViewAfterScreenUpdates:NO];
if (_presentedModals.count < 2) {
_snapshot = [_controller.visibleViewController.view snapshotViewAfterScreenUpdates:NO];
} else {
_snapshot = [[_presentedModals.lastObject view] snapshotViewAfterScreenUpdates:NO];
}
Comment on lines +1178 to +1182
Copy link
Member

@kkafar kkafar May 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I see.
There, most likely, still will be an issue when we dismiss a modal from different stack. I believe you can test this on Test1829 (if not present in FabricTestExample it is in TestsExample).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, it does not work in such case, do we want to handle it right now?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we won't be really able to come up with anything different that a heuristic solution before we do proper modal presenting logic refactor.

Let's proceed now, I'll create note on our task board so that this is not forgotten.

}

- (void)mountingTransactionWillMount:(react::MountingTransaction const &)transaction
Expand Down
Loading