-
Notifications
You must be signed in to change notification settings - Fork 421
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
Add Resources and Style to Popup #1351
Conversation
The app created for verification of this PR has been uploaded to the following location. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI - a unit test is failing https://dev.azure.com/dotnet/CommunityToolkit/_build/results?buildId=97272&view=logs&j=3f96dcf5-6e1e-5485-3200-c557d5216be3&t=885110bc-af55-5c2d-c8fd-8677fc4411e5&l=49
Failed CommunityToolkit.Maui.UnitTests.Views.PopupTests.NullColorThrowsArgumentNullException [8 ms]
Error Message:
System.NullReferenceException : Object reference not set to an instance of an object.
Stack Trace:
at Microsoft.Maui.Controls.MergedStyle.RegisterImplicitStyles()
at Microsoft.Maui.Controls.MergedStyle..ctor(Type targetType, BindableObject target)
at CommunityToolkit.Maui.Views.Popup..ctor() in /_/src/CommunityToolkit.Maui/Views/Popup/Popup.shared.cs:line 68
at CommunityToolkit.Maui.UnitTests.Views.PopupTests.NullColorThrowsArgumentNullException() in /Users/runner/work/1/s/src/CommunityToolkit.Maui.UnitTests/Views/Popup/PopupTests.cs:line 190
at System.RuntimeMethodHandle.InvokeMethod(Object target, Void** arguments, Signature sig, Boolean isConstructor)
at System.Reflection.MethodInvoker.Invoke(Object obj, IntPtr* args, BindingFlags invokeAttr)
@brminnick , Thanks for the unit test results. |
I modified the TargetType passed to the constructor of the MergedStyle class as follows. [When the result obtained by GetType() is Popup]
[When the result obtained by GetType() is not Popup]
[src\CommunityToolkit.Maui\Views\Popup\Popup.shared.cs]
When using the Popup class without inheriting it, when searching for implicit styles with the RegisterImplicitStyles |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this PR, looks good in general, I believe this one will be an easy one to merge. Can you add a sample page showing of this feature?
Co-authored-by: Pedro Jesus <[email protected]>
Co-authored-by: Pedro Jesus <[email protected]>
Co-authored-by: Pedro Jesus <[email protected]>
Co-authored-by: Pedro Jesus <[email protected]>
Co-authored-by: Pedro Jesus <[email protected]>
I would like to think about a sample using the creation of the sample project as a reference. |
…leryViewModel.cs Co-authored-by: Shaun Lawrence <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cat0363 this is looking great! Just a small point on the casting. Otherwise I think we are good!
Thank you so much for this!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we are good! Thanks so much @cat0363
Awesome!! Congrats and thank you @cat0363 for your always awesome work!! The next step is to write the documentation for the new APIs! I've temporarily added the
pending documentation
You can open a PR on the MicrosoftDocs repo here: I'll defer to @bijington to help with any questions you may have 🙌 |
@cat0363 how do you feel about doing the docs? If you like I am more than happy to take on the effort |
@bijington , I wish I could write a document, but I'm embarrassed to say that I don't have the literary talent to write it in fluent English that anyone can understand. If possible, I would like to ask @bijington , who has published an excellent book, to create this document. I read your wonderful book.
@brminnick , Thanks for explaining what the labels mean. |
@cat0363 poease don't be embarrassed, I will happily do it. And if you ever need assistance in this area I'm always willing to help. And wow! Thank you so much for firstly buying the book and secondly for the kind words. I can't tell you how much I appreciate it! |
@bijington , Thank you for taking on the task of creating this document. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @cat0363 and @bijington!!
@bijington , Thank you for creating the document. |
In this PR, we will add Resources and Style to the Popup so that Style can be applied to the Popup.
Description of Change
Since Popup is a class that inherits from Element and not from VisualElement, it does not have Resources and Style.
Modifications were made with reference to the following source code of .NET MAUI.
[src\Controls\src\Core\MergedStyle.cs]
[src\Controls\src\Core\Shell\NavigableElement.cs]
[src\Controls\src\Core\VisualElement\VisualElement.cs]
See above for the main implementation.
Inherited IResourcesProvider to be able to specify Resources for Popup.
[src\CommunityToolkit.Maui\Views\Popup\Popup.shared.cs]
I also added the following implementation:
[src\CommunityToolkit.Maui\Views\Popup\Popup.shared.cs]
The above is ported with reference to the VisualElement class of .NET MAUI.
[src\Controls\src\Core\VisualElement\VisualElement.cs]
This makes it possible to specify Resources in the Popup.
Next, I added the following to make it possible to specify global styles.Next, I added the following to make it possible to specify global styles.
[src\CommunityToolkit.Maui\Views\Popup\Popup.shared.cs]
GetType().BaseType is specified as the first argument of the constructor of MergedStyle, because the style search target within the implementation of the MergedStyle class is the following class.
I referenced the implementation below.
[src\Controls\src\Core\MergedStyle.cs]
[src\Controls\src\Core\Shell\NavigableElement.cs]
Since Popup inherits from Element and Element is the first hit when searching for a class to apply the style, the type of Popup's Base class is specified.
This makes it possible to specify global styles in the Popup.
Next, Inherited IStyleSelectable to allow styles to be applied to the Popup.
[src\CommunityToolkit.Maui\Views\Popup\Popup.shared.cs]
I also added the following implementation:
[src\CommunityToolkit.Maui\Views\Popup\Popup.shared.cs]
I ported it with reference to the following implementation of .NET MAUI.
[src\Controls\src\Core\VisualElement\VisualElement.cs]
The above implementation allows you to specify implicit styles, explicit styles, dynamic styles, and style classes.
Linked Issues
PR Checklist
approved
(bug) orChampioned
(feature/proposal)main
at time of PRAdditional information
Windows results are with PR #1350 applied.
Below is the validation result when specifying the global style.
[App.xaml]
[Popup1.xaml]
Android.Emulator.-.pixel_2_-_api_30_5554.2023-08-17.16-12-19.mp4
iPhone.14.iOS.16.4.2023-08-17.16-36-59.mp4
2023-08-17.16-23-55.mp4
Below is the validation result when specifying an explicit style.
[Popup2.xaml]
Android.Emulator.-.pixel_2_-_api_30_5554.2023-08-17.16-13-37.mp4
iPhone.14.iOS.16.4.2023-08-17.16-37-15.mp4
2023-08-17.16-24-35.mp4
Below is the validation result when specifying the implicit style.
[Popup3.xaml]
Android.Emulator.-.pixel_2_-_api_30_5554.2023-08-17.16-15-52.mp4
iPhone.14.iOS.16.4.2023-08-17.16-37-27.mp4
2023-08-17.16-25-43.mp4
Below is the validation result when specifying the dynamic style.
Android.Emulator.-.pixel_2_-_api_30_5554.2023-08-17.16-17-04.mp4
iPhone.14.iOS.16.4.2023-08-17.16-37-39.mp4
2023-08-17.16-26-23.mp4
Below is the validation result when specifying the style class.
Android.Emulator.-.pixel_2_-_api_30_5554.2023-08-17.16-18-23.mp4
iPhone.14.iOS.16.4.2023-08-17.16-37-51.mp4
2023-08-17.16-29-32.mp4
From the above, you can see that the style can be applied to the Popup as intended.
I would like to ask you to correct the document.