Fix IOS bug in media Element when using Popup#1597
Fix IOS bug in media Element when using Popup#1597pictos merged 24 commits intoCommunityToolkit:mainfrom
Conversation
|
@ne0rrmatrix thank you for submitting this. Sorry to ask this but is there an easy way to test this fix? I'm guessing there isn't anything in the sample that can be tested? |
|
@bijington you can load this PR in the quoted issues example repo. This fix requires targeting dotnet 8.0.x. |
OK thanks. Do you think there is any value in us introducing a similar example in the sample application? I'm happy to assist getting it in as part of the testing. |
Absolutely! Always 💯 Anytime we fix a bug, we should add an example of its reproduction to the Sample App. This helps us verify the bug fix and ensures that we won't have a regression when testing a future PR. |
|
@brminnick I will work on adding reproduction to the sample app this week. I assume adding with PR is a bad idea? I am thinking separate because if we end up reverting PR the example goes away. |
|
Thanks! Yes, please add it to this PR |
|
@brminnick I rewrote some View Controller logic for IOS. This change allows the controls to actually work. I still have a bunch of testing to do so please wait to review. I added a Popup with mediaElement inside it to the Media element page as a button on the far right called Popup in the sample app. |
|
Have done some testing and fixed a bug with Navigation Page logic. I still have to test Flyout Page and Tabbed Page. Once I confirm they are working I will tag someone to review. |
|
Ok I have tested against shell, navigation Page,Tabbed Page, Content Page, and Collection View. Does anyone have any other suggested tests? After testing Collection View I realized this may be fixed too #1421 since testing with collection view worked. Should I add another test in sample for CollectionView? I am not sure where to put it xaml if we want to do that. It is easy to implement but I can't think of a good place to put it in layout. If we are adding another test to sample app I would love help with doing that. I am not sure about layout and how we would want to add collection view to the page. @bijington @brminnick If we are not adding another example to sample for collection view this PR is ready for review by maintainer. |
|
Fix for collection view controller when using Carousel View has been added. Anyone who looks at this for review please indicate whether the approach I use is appropriate. There is now at least one other PR out to fix same issue. I am not sure which approach is appropriate and what the maintainers feel is best to move forward. I am happy either way. |
Description of Change
Linked Issues
PR Checklist
approved(bug) orChampioned(feature/proposal)mainat time of PRAdditional information
Fixes CTD when using media element in a Popup. The incorrect Parent ViewController is being used when adding controls to parent view controller. A sample for popup has been added to samples for media element. This document has been edited since written to better explain the changes and what the fix does.