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

Make sure Popup will set the Window at the Content #674

Merged
merged 8 commits into from
Oct 18, 2022

Conversation

pictos
Copy link
Member

@pictos pictos commented Oct 5, 2022

Description of Change

.NET MAUI has the concept of Window inside some controls, when we implemented the Popup we don't care (or this concept diudn't exist yet) about it, since it should be set automagically, but this isn't the case here because Popup is an Element so we need to help the framework to set it.

Linked Issues

From our Discord channel.

PR Checklist

Additional information

@pictos pictos added the needs discussion Discuss it on the next Monthly standup label Oct 6, 2022
Copy link
Collaborator

@brminnick brminnick left a comment

Choose a reason for hiding this comment

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

Hey Pedro! I just tried the reproduction sample from #629 using the NuGet Package from this PR (CommunityToolkit.Maui.1.0.0-build-674.80100.nupkg), and the iOS Lotte Animation is still frozen inside of the Popup.

Android looks good now!

iOS / macOS Animation in Popup Still Frozen Android (Looks good!)
ScreenFlow ScreenFlow

@pictos
Copy link
Member Author

pictos commented Oct 10, 2022

@brminnick thanks for testing on iOS, I'll give a try here, I hope the tooling works after this update

@pictos
Copy link
Member Author

pictos commented Oct 10, 2022

So... This is kinda funny, and I'm not sure if the bug is here or in .NET MAUI.
I downloaded the sample and added our .Core and .Maui projects. If I open the popup without a breaking point, the Lottie animation doesn't work, now if I set a break point and hit play it works.

So looks like a timing issue(?)

cc: @PureWeen

PopupLottie.mp4

@pictos pictos added the blocked label Oct 17, 2022
@pictos pictos removed the blocked label Oct 17, 2022
@brminnick
Copy link
Collaborator

@pictos - Can we merge this since it fixes the bug on Android? As long as we keep the Bug open, we should be good right?

@pictos
Copy link
Member Author

pictos commented Oct 17, 2022

@brminnick yes, we can. I'll create a new issue related to the Window property, and link this PR to it. I'll also do a couple more changes on the PR during this week, I found some places where this can be improved.

brminnick
brminnick previously approved these changes Oct 17, 2022
Copy link
Collaborator

@brminnick brminnick left a comment

Choose a reason for hiding this comment

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

Approved bug fix for Android

@pictos pictos merged commit ba22c8e into main Oct 18, 2022
@pictos pictos deleted the pj/popup-window-implementation branch October 18, 2022 17:21
@brminnick brminnick removed the needs discussion Discuss it on the next Monthly standup label Nov 3, 2022
@brminnick brminnick added this to the v1.4.0 milestone Nov 3, 2022
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.

[Housekeeping] Make sure Popup pass the Window from its Parent to Popup.Content
2 participants