Handlers should not implement IImageSourcePartSetter directly
#16901
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mattleibow
changed the title
IImageSourcePartSetter directly
mattleibow
commented
Aug 21, 2023
Comment on lines
-15
to
+16
| public partial class ButtonHandler : IButtonHandler, IImageSourcePartSetter | ||
| public partial class ButtonHandler : IButtonHandler |
There was a problem hiding this comment.
Removed the interface from the handler here.
Comment on lines
+20
to
+21
| public virtual ImageSourcePartLoader ImageSourceLoader => | ||
| _imageSourcePartLoader ??= new ImageSourcePartLoader(new ButtonImageSourcePartSetter(this)); |
There was a problem hiding this comment.
Made this virtual so derived types can actually do something different.
| static Microsoft.Maui.Handlers.SwipeItemMenuItemHandler.MapSource(Microsoft.Maui.Handlers.ISwipeItemMenuItemHandler! handler, Microsoft.Maui.ISwipeItemMenuItem! view) -> void | ||
| static Microsoft.Maui.Handlers.SwipeItemMenuItemHandler.MapSource(Microsoft.Maui.Handlers.ISwipeItemMenuItemHandler! handler, Microsoft.Maui.ISwipeItemMenuItem! image) -> void |
There was a problem hiding this comment.
Vote on this. The param name changed so I am letting it slide, but technically breaking if the dev used named params, but that is a bit hard to do in this case.
The alternative is to duplicate the code in windows with a new param name...
mattleibow
commented
Aug 21, 2023
src/Core/src/Handlers/SwipeItemMenuItem/ISwipeItemMenuItemHandler.cs
Outdated
Show resolved
Hide resolved
PureWeen
reviewed
Aug 21, 2023
hartez
previously approved these changes
Aug 22, 2023
PureWeen
previously approved these changes
Aug 22, 2023
|
/rebase |
PureWeen
approved these changes
Aug 23, 2023
rachelkang
approved these changes
Aug 23, 2023
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description of Change
Handlers should not implement the
IImageSourcePartSetterinterface directly because this prevents any customization as well as does not support handlers with multiple image sources.As of today, there is no handler yet with multiple image sources and technically we can implement the interface directly on the handler, but it is not best practice. A hander should be re-usable and connecting a specific implementation to a handler is not recommended.
This PR does a few things:
IImageSourcePartSetterinterface from the handlers (button, image, image button and swipe items)ImageSourcePartLoader SourceLoader { get; }to theISwipeItemMenuItemHandlerhandler interfaceIImageSourcePartSettertype