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

[iOS] Use UIButtonType.System for Button and utilize UIButton.Appearance #554

Merged
merged 14 commits into from Mar 20, 2017

Conversation

Projects
None yet
6 participants
@adrianknight89
Contributor

adrianknight89 commented Nov 21, 2016

Description of Change

UIButtonType.RoundedRect was deprecated in iOS 7 and replaced by UIButtonType.System. Also, when buttons are being set up, the renderer now accounts for UIButton.Appearance proxy. I did not change the button type to Custom since it changes default behavior as explained here.

Bugs Fixed

PR Checklist

  • Has tests (if omitted, state reason in description)
  • Rebased on top of master at time of PR
  • Changes adhere to coding standard
  • Consolidate commits as makes sense
Show outdated Hide outdated ...ms.Controls.Issues/Xamarin.Forms.Controls.Issues.Shared/Bugzilla40251.cs
// Apply the default category of "Issues" to all of the tests in this assembly
// We use this as a catch-all for tests which haven't been individually categorized
#if UITEST
[assembly: NUnit.Framework.Category("Issues")]

This comment has been minimized.

@hartez

hartez Dec 27, 2016

Member

This only needs to be set once for the assembly; you can remove it from this class.

@hartez

hartez Dec 27, 2016

Member

This only needs to be set once for the assembly; you can remove it from this class.

This comment has been minimized.

@adrianknight89

adrianknight89 Dec 29, 2016

Contributor

Should it also be removed from _Template.cs?

@adrianknight89

adrianknight89 Dec 29, 2016

Contributor

Should it also be removed from _Template.cs?

Show outdated Hide outdated Xamarin.Forms.Platform.iOS/Renderers/ButtonRenderer.cs
@@ -88,6 +90,19 @@ protected override void OnElementPropertyChanged(object sender, PropertyChangedE
UpdateImage();
}
void SetControlPropertiesFromProxy()
{
var controlStates = new[] {UIControlState.Normal, UIControlState.Highlighted, UIControlState.Disabled};

This comment has been minimized.

@hartez

hartez Dec 27, 2016

Member

Can this array be made static on the ButtonRenderer class, rather than creating a new instance every time?

@hartez

hartez Dec 27, 2016

Member

Can this array be made static on the ButtonRenderer class, rather than creating a new instance every time?

};
}
}
}

This comment has been minimized.

@hartez

hartez Dec 27, 2016

Member

Since this isn't an automated test, please add some instructions for the reviewer so they can determine if it's passing or not. Also, this doesn't really seem to test the changed behavior; could this test be modified to update the proxy appearance and then create a new button? That way we're demonstrating what the change does and providing a code example.

@hartez

hartez Dec 27, 2016

Member

Since this isn't an automated test, please add some instructions for the reviewer so they can determine if it's passing or not. Also, this doesn't really seem to test the changed behavior; could this test be modified to update the proxy appearance and then create a new button? That way we're demonstrating what the change does and providing a code example.

This comment has been minimized.

@adrianknight89

adrianknight89 Jan 8, 2017

Contributor

Done.

@adrianknight89

adrianknight89 Jan 8, 2017

Contributor

Done.

Show outdated Hide outdated Xamarin.Forms.ControlGallery.iOS/AppDelegate.cs
@@ -162,6 +162,8 @@ public override bool FinishedLaunching(UIApplication uiApplication, NSDictionary
MessagingCenter.Subscribe<NativeBindingGalleryPage>(this, NativeBindingGalleryPage.ReadyForNativeBindingsMessage, AddNativeBindings);
LoadApplication(app);
// uncomment to set default button title color for normal state
//UIButton.Appearance.SetTitleColor(UIColor.Red, UIControlState.Normal);

This comment has been minimized.

@adrianknight89

adrianknight89 Dec 29, 2016

Contributor

@hartez I had added this line to update the proxy appearance, but I commented it out so that the button title color is not red across the gallery app. What would be another way to do this so that the test is scoped to 40251? Perhaps use a custom renderer?

@adrianknight89

adrianknight89 Dec 29, 2016

Contributor

@hartez I had added this line to update the proxy appearance, but I commented it out so that the button title color is not red across the gallery app. What would be another way to do this so that the test is scoped to 40251? Perhaps use a custom renderer?

This comment has been minimized.

@hartez

hartez Dec 30, 2016

Member

Custom renderer would probably work. Another option might be to use MessagingCenter to communicate from the test to AppDelegate and update the proxy appearance, then reset it after the test has run. I'm not sure whether that would work, but it might be easier than creating a custom renderer.

@hartez

hartez Dec 30, 2016

Member

Custom renderer would probably work. Another option might be to use MessagingCenter to communicate from the test to AppDelegate and update the proxy appearance, then reset it after the test has run. I'm not sure whether that would work, but it might be easier than creating a custom renderer.

@hartez hartez self-assigned this Dec 30, 2016

Show outdated Hide outdated Xamarin.Forms.Platform.iOS/Renderers/ButtonRenderer.cs
Control.SetTitleColor(UIButton.Appearance.TitleColor(uiControlState), uiControlState); // if new values are null, old values are preserved.
Control.SetTitleShadowColor(UIButton.Appearance.TitleShadowColor(uiControlState), uiControlState);
Control.SetBackgroundImage(UIButton.Appearance.BackgroundImageForState(uiControlState), uiControlState);
Control.SetImage(UIButton.Appearance.ImageForState(uiControlState), uiControlState); // UpdateImage() renders this useless.

This comment has been minimized.

@adrianknight89

adrianknight89 Jan 8, 2017

Contributor

@hartez I chose the custom renderer approach. The code is going to test title color, title shadow color, and background image. It looks like setting image doesn't work because UpdateImage() overwrites anything set before. Let me know if the 4th line should be removed.

Note that UpdateImage() sets image to null only for the normal state. Also setting image seems to require more work such as setting content mode and computing edge insets.

@adrianknight89

adrianknight89 Jan 8, 2017

Contributor

@hartez I chose the custom renderer approach. The code is going to test title color, title shadow color, and background image. It looks like setting image doesn't work because UpdateImage() overwrites anything set before. Let me know if the 4th line should be removed.

Note that UpdateImage() sets image to null only for the normal state. Also setting image seems to require more work such as setting content mode and computing edge insets.

This comment has been minimized.

@hartez

hartez Jan 17, 2017

Member

Go ahead and take out the SetImage line for now; I have an idea of how we can handle that, but we can create another PR for that.

The BackgroundImage test is crashing when I run it in the simulator - it looks like you're referencing image.png in the custom renderer, but that image doesn't seem to be in the project.

@hartez

hartez Jan 17, 2017

Member

Go ahead and take out the SetImage line for now; I have an idea of how we can handle that, but we can create another PR for that.

The BackgroundImage test is crashing when I run it in the simulator - it looks like you're referencing image.png in the custom renderer, but that image doesn't seem to be in the project.

This comment has been minimized.

@adrianknight89

adrianknight89 Jan 17, 2017

Contributor

Not sure what went wrong with the image, but I replaced it with an existing one.

@adrianknight89

adrianknight89 Jan 17, 2017

Contributor

Not sure what went wrong with the image, but I replaced it with an existing one.

Show outdated Hide outdated Xamarin.Forms.Platform.iOS/Renderers/ButtonRenderer.cs
Control.SetTitleColor(UIButton.Appearance.TitleColor(uiControlState), uiControlState); // if new values are null, old values are preserved.
Control.SetTitleShadowColor(UIButton.Appearance.TitleShadowColor(uiControlState), uiControlState);
Control.SetBackgroundImage(UIButton.Appearance.BackgroundImageForState(uiControlState), uiControlState);
Control.SetImage(UIButton.Appearance.ImageForState(uiControlState), uiControlState); // UpdateImage() renders this useless.

This comment has been minimized.

@hartez

hartez Jan 17, 2017

Member

Go ahead and take out the SetImage line for now; I have an idea of how we can handle that, but we can create another PR for that.

The BackgroundImage test is crashing when I run it in the simulator - it looks like you're referencing image.png in the custom renderer, but that image doesn't seem to be in the project.

@hartez

hartez Jan 17, 2017

Member

Go ahead and take out the SetImage line for now; I have an idea of how we can handle that, but we can create another PR for that.

The BackgroundImage test is crashing when I run it in the simulator - it looks like you're referencing image.png in the custom renderer, but that image doesn't seem to be in the project.

@hartez

This comment has been minimized.

Show comment
Hide comment
@hartez

hartez Jan 23, 2017

Member

@adrianknight89 This is looking good, can you rebase it? It needs the updated .pfx files from master so I can run it through the CI system again.

Member

hartez commented Jan 23, 2017

@adrianknight89 This is looking good, can you rebase it? It needs the updated .pfx files from master so I can run it through the CI system again.

@adrianknight89

This comment has been minimized.

Show comment
Hide comment
@adrianknight89

adrianknight89 Jan 23, 2017

Contributor

Done.

Contributor

adrianknight89 commented Jan 23, 2017

Done.

@hartez

hartez approved these changes Jan 31, 2017

@hartez hartez requested a review from jassmith Feb 1, 2017

@rmarinho rmarinho merged commit ea4673c into xamarin:master Mar 20, 2017

@samhouts samhouts added D-15.4 and removed cla-already-signed labels Oct 10, 2017

@samhouts samhouts added this to the 2.3.5 milestone Jun 27, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment