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

RadioButton Implementation #8910

Merged
merged 10 commits into from
Feb 18, 2020
Merged

RadioButton Implementation #8910

merged 10 commits into from
Feb 18, 2020

Conversation

jfversluis
Copy link
Member

@jfversluis jfversluis commented Dec 16, 2019

Description of Change

Takeover from the RadioButton PR in #5349. I've copied the relevant parts here.

Issues Resolved

API Changes

Added:

public class RadioButton
{
      public event EventHandler Checked;
      public event EventHandler Unchecked;

      public bool IsChecked {get; set; }  //BP
      public string GroupName { get; set; } //BP
      public ImageSource ButtonSource { get; set; } //BP
}

Platforms Affected

  • Core/XAML (all platforms)
  • iOS
  • Android
  • UWP
  • MacOS

Behavioral/Visual Changes

GroupName property

The "RadioButton group Gallery - Legacy" has three tabs which show group behavior.
Radio-buttons with group name null are grouped by their parent.
Radio-buttons with same group name, no matter the hierarchy, are grouped at page level.

Video: https://www.youtube.com/watch?v=fwYSmvjCfbw

Before/After Screenshots

image

Testing Procedure

See the newly added gallery pages

PR Checklist

  • Targets the correct branch
  • Tests are passing (or failures are unrelated)

TODO

  • Make Mac OS work with the groups, you can now check all radiobuttons, group seems to be ignored completely

image

  • on iOS you can uncheck a radiobutton by tapping it a second time, this should not happen

@jfversluis
Copy link
Member Author

Need to have a look at the review comments in the original PR

Xamarin.Forms.Platform.iOS/Renderers/RadioButonCALayer.cs Outdated Show resolved Hide resolved
Xamarin.Forms.Platform.iOS/Renderers/RadioButonCALayer.cs Outdated Show resolved Hide resolved
Xamarin.Forms.Core/RadioButton.cs Show resolved Hide resolved
Xamarin.Forms.Platform.Android/IButtonLayoutRenderer.cs Outdated Show resolved Hide resolved
Xamarin.Forms.Platform.iOS/Renderers/RadioButonCALayer.cs Outdated Show resolved Hide resolved
Xamarin.Forms.Platform.UAP/FormsRadioButton.cs Outdated Show resolved Hide resolved
Copy link
Member

@rmarinho rmarinho left a comment

Choose a reason for hiding this comment

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

  • What does ButtonSource do? can we add the image to UWP and iOS so we can test it with VSM in all platforms?

  • Padding seems to be applied only on text, not the all control viewport.

image

  • Can we add a UITests for iOS and Android

  • On UWP when i hover the border goes away.

  • Is there a way to change the color of the Radio (Circle) on UWP ?! seems we can only change the TextColor, but on iOS seems TextColor changes color of text and circle


SetControlPropertiesFromProxy();

_useLegacyColorManagement = e.NewElement.UseLegacyColorManagement();
Copy link
Member

Choose a reason for hiding this comment

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

If we have a new Control why we are checking for LegacyColorManagement?

}
);

var borderRadiusContainer = new ViewContainer<RadioButton>(Test.Button.BorderRadius,
Copy link
Member

@rmarinho rmarinho Jan 7, 2020

Choose a reason for hiding this comment

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

This should be CornerRadius no ? but it also doesn't work for me if i change to something like:

	var borderRadiusContainer = new ViewContainer<RadioButton>(Test.Button.BorderRadius,
				new RadioButton
				{
					Text = "BorderRadius",
					BackgroundColor = Color.Transparent,
					BorderColor = Color.Red,
					BorderWidth = 1,
					CornerRadius = 5
				}
			);

@rmarinho rmarinho self-requested a review February 18, 2020 16:31
@rmarinho rmarinho merged commit 91d30c9 into master Feb 18, 2020
@samhouts samhouts added the approved Has two approvals, no pending reviews, and no changes requested label Feb 18, 2020
@jfversluis jfversluis deleted the radiobutton branch February 19, 2020 19:45
@samhouts samhouts added this to the 4.6.0 milestone Feb 28, 2020
namespace Xamarin.Forms
{
[RenderWith(typeof(_RadioButtonRenderer))]
public class RadioButton : Button, IElementConfiguration<RadioButton>
Copy link
Collaborator

Choose a reason for hiding this comment

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

why don't you implements IFontElement, ITextElement, IBorderElement instead of inheriting Button directly.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
a/radiobutton 🔘 a/visual API-change Heads-up to reviewers that this PR may contain an API change approved Has two approvals, no pending reviews, and no changes requested ControlGallery Core F100 p/Android p/iOS 🍎 p/macOS p/UWP proposal-open roadmap t/enhancement ➕
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[F100] RadioButton
5 participants