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

Fix StyleSheet 'textAlign' for AndroidTextInput,rename Android native props #4364

Closed
wants to merge 1 commit into from

Conversation

hyugit
Copy link

@hyugit hyugit commented Nov 26, 2015

  • rename Android native prop 'textAlign' to 'textAlignAndroid'
  • rename Android native prop 'textAlignVertical' to 'textAlignVerticalAndroid'
  • add another 'setTextAlign' in ReactTextInputManager for StyleSheet prop
  • add demo

…e prop

- rename Android native prop 'textAlign' to 'textAlignAndroid'
- rename Android native prop 'textAlignVertical' to 'textAlignVerticalAndroid'
- add another 'setTextAlign' in ReactTextInputManager for StyleSheet prop
- add demo
@facebook-github-bot
Copy link
Contributor

By analyzing the blame information on this pull request, we identified @foghina, @kmagiera and @mkonicek 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 26, 2015
@hyugit
Copy link
Author

hyugit commented Nov 26, 2015

The reason behind renaming 'textAlign' to 'textAlignAndroid': StyleSheet is looking for a prop 'textAlign' of type 'String', while AndroidTextInput is looking for an Android native prop with the same name of type 'int'

@foghina
Copy link
Contributor

foghina commented Nov 26, 2015

@andreicoman11

@kmagiera
Copy link
Contributor

I don't understand why we need separate textAlign attribute at all. Can't we just update all android TextInput examples to set textAlign through style property and also add textVerticalAlign to the StyleSheet definition instead of exposing it as a direct property of the view (it may be implemented for regular textview as well).

cc @foghina who may have more context on why the decision on not using textAlign from StyleSheet was made here 22ea669

@hyugit
Copy link
Author

hyugit commented Dec 1, 2015

Hi @kmagiera, thanks for your input. It seems that ReactEditText and ReactTextView are inherited from different native views: ReactEditText extends EditText, ReactTextView extends TextView. And the setTextAlign method I added are almost identical to the one in TextView, but since they are from different super classes, I'm not sure how to create a simpler and more elegant solution.

And with the changes in this PR, StyleSheet will be picking up textAlign of type String.

@kmagiera
Copy link
Contributor

kmagiera commented Dec 1, 2015

I think it's fine to use Strings for textAlign property especially that TextView is a component that is used much more frequently and considering that it's using Strings for this property means that using int in EditText shouldn't make that much of a difference.

To actually have any benefits from optimizing this sort of bridge traffic it would be better to think about some more generic solution that would allow us to perform this type of String<->int conversions that would be more transparent from the code perspective, so that we can optimize all those properties automatically.

@hyugit
Copy link
Author

hyugit commented Dec 1, 2015

BTW, is it possible to overload @ReactMethod methods?

@kmagiera
Copy link
Contributor

kmagiera commented Dec 1, 2015

Yes, although need to add @ReactNative annotation to the overloaded method as well

@hyugit
Copy link
Author

hyugit commented Dec 1, 2015

@kmagiera I just put up a new PR #4481 that addresses some of the issues we talked about here. Basically, the new PR is no longer a breaking change, and it also unifies the Android native prop and the StyleSheet prop.

@kmagiera
Copy link
Contributor

kmagiera commented Dec 2, 2015

Thanks so much, I understand this can now be closed in favor of #4481

@kmagiera kmagiera closed this Dec 2, 2015
@hyugit hyugit deleted the master branch December 3, 2015 08:04
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants