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

[Switch] Remove fixed size for Android. Fixes #3785 #4298

Closed
wants to merge 1 commit into from

Conversation

christopherdro
Copy link
Contributor

Fixed #3785

@facebook-github-bot
Copy link
Contributor

By analyzing the blame information on this pull request, we identified @sahrens and @kmagiera to be potential reviewers.

@facebook-github-bot facebook-github-bot added GH Review: review-needed CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. labels Nov 23, 2015
@christopherdro
Copy link
Contributor Author

cc @mkonicek

@mkonicek
Copy link
Contributor

mkonicek commented Dec 2, 2015

I think this is good! cc @sahrens

How have you tested it? Can you add a screenshot from the UIExplorer for example?

@christopherdro
Copy link
Contributor Author

@mkonicek Yea I've tested it.
Here is a screenshot from UIExplorer using Switch instead of Switch Android.

wo-changes

With change matches: SwitchAndroid

w-changes

@sahrens
Copy link
Contributor

sahrens commented Dec 5, 2015

Can we please not have different layout behavior between iOS and android? This makes android switch stretch and right align which is super weird and does not match iOS.

@sahrens
Copy link
Contributor

sahrens commented Dec 5, 2015

Can we just change the width? Or pass down the width from the native module if it varies by device?

@christopherdro
Copy link
Contributor Author

@sahrens I had originally thought about just adjusting the width but after speaking with @mkonicek he mentioned something about auto-layout with Android and that it's not necessary to define and fixed values.

Adjust the width to 45 makes it not get cut off but all the switches are still aligned left.

Does the switch component vary in size between different Android devices?

@kmagiera
Copy link
Contributor

kmagiera commented Dec 6, 2015

Size of switch on android varies between OS versions, it can be different when some of the accessibility features are on AFAIK, not to mention custom skins/themes some vendor like to preinstall. It's always been measuring itself because of the aforementioned reasons and I personally support this change. Not sure what the benefit would be from having it send over but it will definitely introduce some limitations (e.g currently we can start initialising the bridge before activity is started and we don't yet have access to the theme at that time, to pass it down as a constant we'd need to have theme before we initialise the bridge)

@sahrens
Copy link
Contributor

sahrens commented Dec 7, 2015

If it's not reasonable to pass the size through as a constant, there are two options to make it consistent with iOS:

  1. make it not stretch to fill the parent (it should end up left aligned in the example).

  2. change iOS to stretch and fill the parent and right-align in the example like android (and fix all the existing usage of switch in our iOS apps so we don't break anything).

The only thing I really care about is consistency.

@christopherdro
Copy link
Contributor Author

The current changes should keep the same layout behavior if you were to use SwitchIOS or SwitchAndroid separately since SwitchIOS is the only one that has styling pre defined and passed into the native component.

@sahrens
Copy link
Contributor

sahrens commented Dec 8, 2015

But are they interchangeable? We should strive for interchangeable whenever it's reasonable, and this seems like a perfect case for that.

@mkonicek
Copy link
Contributor

mkonicek commented Dec 9, 2015

I like what Spencer suggested:

  1. make it not stretch to fill the parent (it should end up left aligned in the example).

Is that doable? I think you need to change the measure logic in Java. I agree it's a bit strange the Android Switch fills parent and is right-aligned.

@mkonicek mkonicek assigned mkonicek and unassigned dmmiller Dec 9, 2015
@satya164
Copy link
Contributor

@christopherdro Would love to get this merged. Without this the Cross platform switch component is not usable.

@mkonicek
Copy link
Contributor

mkonicek commented Jan 6, 2016

Let's figure out the alignment on Android separately. I agree without this PR the cross-platform switch is unusable.

@mkonicek
Copy link
Contributor

mkonicek commented Jan 6, 2016

@facebook-github-bot shipit

@facebook-github-bot
Copy link
Contributor

Thanks for importing. If you are an FB employee go to https://our.intern.facebook.com/intern/opensource/github/pull_request/179289565750654/int_phab to review.

christopherdro added a commit to wildlifela/react-native that referenced this pull request Jan 20, 2016
Summary:
Fixed facebook#3785
Closes facebook#4298

Reviewed By: svcscm

Differential Revision: D2807289

Pulled By: mkonicek

fb-gh-sync-id: 6f161e4f8b04597726183fdcf8bc22c682557958
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. JavaScript
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants