Skip to content
This repository was archived by the owner on May 1, 2024. It is now read-only.

Conversation

PureWeen
Copy link
Contributor

Description of Change

ImageButton Renderer for Android didn't provide overrides that are consistent with other renderers for hooking into element and element property change events

API Changes

Added the following methods to the ImageButtonRenderer

protected virtual void OnElementChanged(ElementChangedEventArgs<ImageButton> e);
protected virtual void OnElementPropertyChanged(object sender, PropertyChangedEventArgs e);

Platforms Affected

  • Android

Testing Procedure

Add a custom ImageButtonRenderer for Android

PR Checklist

  • Has automated tests
  • Rebased on top of the target branch at time of PR
  • Changes adhere to coding standard

Copy link
Member

@StephaneDelcroix StephaneDelcroix left a comment

Choose a reason for hiding this comment

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

make the virtual method empty. think about your future children

ImageButton?.SendViewInitialized(Control);
}

protected virtual void OnElementChanged(ElementChangedEventArgs<ImageButton> e)
Copy link
Member

Choose a reason for hiding this comment

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

it's better to keep the virtual void OnElementChanged empty and invokes it after firing the event

Copy link
Contributor Author

@PureWeen PureWeen Nov 21, 2018

Choose a reason for hiding this comment

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

I mainly did it like this because this is how VisualElementRenderer does it
https://github.com/xamarin/Xamarin.Forms/blob/master/Xamarin.Forms.Platform.Android/VisualElementRenderer.cs#L305

So I just copied that one for the sake of consistency

Copy link
Contributor

Choose a reason for hiding this comment

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

I suppose it's possible for someone to want to control when (or whether) to invoke ElementChanged when they override OnElementChanged.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For the record I do agree with @StephaneDelcroix

But I did it this way because this is the same way we do it literally everywhere that I could find. So I feel like at this point consistency should be the winner

https://github.com/xamarin/Xamarin.Forms/blob/master/Xamarin.Forms.Platform.iOS/Renderers/WkWebViewRenderer.cs#L126

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, agreed. Consistency is best. :/

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, agreed. Consistency is best. :/

disagreed, but I'm not going to die on this hill

@StephaneDelcroix StephaneDelcroix merged commit 07c9ac7 into 3.4.0 Nov 22, 2018
@StephaneDelcroix StephaneDelcroix deleted the android-renderer-overrides branch November 22, 2018 09:31
@samhouts samhouts added this to the 4.0.0 milestone Dec 4, 2018
@samhouts samhouts modified the milestones: 4.0.0, 3.4.0 Jan 2, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants