Skip to content
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] change checkbox to use default constructor #6512

Merged
merged 1 commit into from Jun 14, 2019
Merged

Conversation

PureWeen
Copy link
Contributor

@PureWeen PureWeen commented Jun 13, 2019

Description of Change

Change the FormsCheckbox inheriting from UIBUtton to not use the UIButtonType base constructor which isn't recommended. If you use the UIButtonType base constructor it throws exceptions. This PR changes it to use the default constructor

Platforms Affected

  • iOS

Testing Procedure

Bring up the CheckBox in the dynamic control gallery and play with the colors and various other settings to make sure it still all works correctly

PR Checklist

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

@PureWeen PureWeen added p/iOS 🍎 blocker Issue blocks next stable release. Prioritize fixing and reviewing this issue. labels Jun 13, 2019
@PureWeen PureWeen added this to the 4.1.0 milestone Jun 13, 2019
@PureWeen PureWeen requested a review from paymicro June 13, 2019 05:57
@PureWeen PureWeen added this to To do in v4.1.0 via automation Jun 13, 2019
@paymicro
Copy link
Contributor

It seems to me that the checkbox is out of the general style, because 30% less than native controls
Screenshot_1

@PureWeen
Copy link
Contributor Author

@paymicro yea the default one could probably stand to be increased in size a bit.

Did you compare the material ones between ios and Android by chance? The sizes I picked were really just to match android.

@jamesmontemagno jamesmontemagno requested review from jamesmontemagno and removed request for jamesmontemagno June 13, 2019 17:12
@jamesmontemagno
Copy link
Contributor

Agreed on size. I think mine was a bit bigger originally?

@jamesmontemagno
Copy link
Contributor

This change looks fine. I don't remembering doing that though, but looks solid change.

@PureWeen
Copy link
Contributor Author

@jamesmontemagno

Agreed on size. I think mine was a bit bigger originally?

Yea this was a little bit of a tricky one. Originally you had the HeightRequest on ios wired up to the drawing so it made a larger checkbox. Problem is that it didn't work on any of the other platforms and there's not a super straight forward way to make it resize on Android. Maybe down the road @paymicro could work the same magic he did on slider :-) so for now it all just sizes to a made up default that was just picked to match android. Things like the Switch and those also don't resize so should checkbox? So that leaves us with just making up some height that matches a made up control on iOS. Switch seems like a valid control to size against. Maybe just match the height of a switch and use that for the wxh

This change looks fine. I don't remembering doing that though, but looks solid change.

Original implementation just inherited from UIView but I changed it to UIButton to be more consistent with how other platforms do it and also this allows us to easily add text

@samhouts samhouts moved this from To do to In Review in v4.1.0 Jun 13, 2019
@samhouts samhouts requested a review from hartez June 13, 2019 18:44
@PureWeen
Copy link
Contributor Author

alright @paymicro and @jamesmontemagno I increased the size of the default checkbox to just match the height of the switch control

I also just setup DefaultSize as a settable property so if someone creates a custom ios renderer they can easily change the height

image

@paymicro
Copy link
Contributor

for full compliance, it remains to add the same shadows as on the switch control

@PureWeen
Copy link
Contributor Author

@paymicro can you log an issue for the shadow?

@jamesmontemagno
Copy link
Contributor

Should there be a shadow? hard to say... i mean looking at iOS apps there aren't at all. This control we are sort of making up for iOS based on apps like Reminder.

@paymicro
Copy link
Contributor

@jamesmontemagno by default the switch circle has a shadow
image
but there are more important things to do...

@jamesmontemagno
Copy link
Contributor

AHHHHHHHH I see what you mean now, this wasn't for checkbox... i was confused.

@hartez hartez merged commit ab11060 into 4.1.0 Jun 14, 2019
v4.1.0 automation moved this from In Review to Done Jun 14, 2019
@samhouts samhouts added the approved Has two approvals, no pending reviews, and no changes requested label Jun 18, 2019
@PureWeen PureWeen deleted the ios_checkbox branch September 2, 2019 18:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Has two approvals, no pending reviews, and no changes requested blocker Issue blocks next stable release. Prioritize fixing and reviewing this issue. p/iOS 🍎
Projects
No open projects
v4.1.0
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

5 participants