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

[Android] Adding Hyphenation Frequency prop for Text component #29157

Closed
wants to merge 8 commits into from

Conversation

fabOnReact
Copy link
Contributor

@fabOnReact fabOnReact commented Jun 17, 2020

Summary

This issue fixes #28279
android_hyphenationFrequency prop for Android Text component which sets the frequency of automatic hyphenation to use when determining word breaks.

Changelog

[Android] [Fixed] - Adding Hyphenation Frequency prop for Text component

Test Plan

More info are available in the android docs. I will add the documentation to the docs later once the pull request is taken in consideration for merging.

AFTER

I remain available to do improvements. Thanks a lot. Fabrizio.

@fabOnReact fabOnReact requested a review from shergin as a code owner June 17, 2020 15:53
@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jun 17, 2020
@react-native-bot react-native-bot added Bug Platform: Android Android applications. labels Jun 17, 2020
@analysis-bot
Copy link

analysis-bot commented Jun 17, 2020

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 6,764,177 2,599
android hermes armeabi-v7a 6,426,979 2,183
android hermes x86 7,150,511 1,794
android hermes x86_64 7,040,444 1,748
android jsc arm64-v8a 8,937,601 2,633
android jsc armeabi-v7a 8,592,787 2,221
android jsc x86 8,767,045 1,815
android jsc x86_64 9,342,605 1,758

Base commit: e75557b

@analysis-bot
Copy link

analysis-bot commented Jun 17, 2020

Platform Engine Arch Size (bytes) Diff
ios - universal 829440 n/a

Base commit: e75557b

@elicwhite
Copy link
Member

I can't confirm that this will get approved (I don't have context on the Android side), but we've been following a pattern of the platform prefixes being android_, so this prop should be android_hyphenationFrequency. This is also seen in these props on Pressable: https://github.com/facebook/react-native/blob/master/Libraries/Components/Pressable/Pressable.js#L122-L127

@fabOnReact
Copy link
Contributor Author

thanks a lot @TheSavior . Changes are included in 4500095. I'm fixing the same issue also in #29059. I have no prerogative for the PR to be merged, but just hope to get a better insight in the react-native project and write more relevant pr in the future.. Thanks a lot. Fabrizio

@JoshuaGross
Copy link
Contributor

This seems pretty reasonable to me :)

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@JoshuaGross has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@JoshuaGross
Copy link
Contributor

@fabriziobertoglio1987 I'm working on testing internally and having some conversations about the prefix. While I'm doing that, could you issue a PR to the docs site to add documentation for the new prop?

https://github.com/facebook/react-native-website/tree/master/docs

@fabOnReact
Copy link
Contributor Author

fabOnReact commented Jun 26, 2020

UPDATE 30/06/2020

I notice now that 0fda91f does not include the latest changes. Tomorrow I will work on testing this functionality on Android API < 23 and add changes from 14709f8 to master if required (in a separate pull request). Additionally I noticed my errors in #29070. Sorry for this, I did a mistake. I will fix #29070 and not make any errors in my future pull request. Thanks a lot and sorry for disturbing. I understand now that ReactNative Core team is very busy and I should deliver higher quality code.

OLD MESSAGE 26/06/2020

Thanks a lot @JoshuaGross I added the docs and also added a new commit to this Pull Request 14709f8 as setHyphenationFrequency android method is available only from API 23+.

I decided to write this PR as a solution of #28279 to give users more control on the Text Component. I don't work on iOS and I had the impression that textBreakingStrategy was related to breaking paragraphs and sentences (justifying a different prop and setter).

I tested on RNTester example emulator API 28.

#28940 unluckily denies me to test this changes on my API 19 emulator. I am on cellular data right now and can not download another Android 22 image to test the code changes from 14709f8 on devices < Android 23, but I believe the logic will work as the same logic is used with textBreakingStrategy

@ReactProp(name = ViewProps.TEXT_BREAK_STRATEGY)
public void setTextBreakStrategy(@Nullable String textBreakStrategy) {
if (Build.VERSION.SDK_INT < Build.VERSION_CODES.M) {
return;
}

I'll work on this and let you know. I remain available. Thanks a lot. I wish you a good day. Fabrizio

@react-native-bot
Copy link
Collaborator

This pull request was successfully merged by @fabriziobertoglio1987 in 0fda91f.

When will my fix make it into a release? | Upcoming Releases

@react-native-bot react-native-bot added the Merged This PR has been merged. label Jun 30, 2020
@fabOnReact fabOnReact deleted the hyphenation-frequency branch July 1, 2020 07:23
@JoshuaGross
Copy link
Contributor

@fabriziobertoglio1987 Thank you for following up on this! The amendment is being merged in.

facebook-github-bot pushed a commit that referenced this pull request Jul 2, 2020
Summary:
JoshuaGross This issue fixes #28279 as discussed in #29157 (comment)
Avoid calling [setHyphenationFrequency](https://developer.android.com/reference/android/widget/TextView#setHyphenationFrequency(int)) on Android Sdk < 23.

## Changelog

<!-- Help reviewers and the release process by writing your own changelog entry. For an example, see:
https://github.com/facebook/react-native/wiki/Changelog
-->

[Android] [Fixed] - do not call setHyphenationFrequency on AndroidSdk < 23
Pull Request resolved: #29258

Test Plan:
| **BEFORE** | **AFTER** |
|:-------------------------:|:-------------------------:|
|  <img src="https://user-images.githubusercontent.com/24992535/86214122-05bf0e00-bb7b-11ea-93b5-2174812bfec9.png"  width="300" height="" />| <img src="https://user-images.githubusercontent.com/24992535/86214130-08216800-bb7b-11ea-9fc0-68b28638bf57.png" width="300" height="" /> |

The warning displayed with `adb logcat | grep -P "ReactTextAnchorViewManager"`

![image](https://user-images.githubusercontent.com/24992535/86214242-34d57f80-bb7b-11ea-9945-30ae25332bfb.png)

I remain available to do improvements. Thanks a lot. Fabrizio.

Reviewed By: JoshuaGross

Differential Revision: D22337095

Pulled By: mdvacca

fbshipit-source-id: d0943397af180929c48044ccbc7a9388549021b8
@fabOnReact
Copy link
Contributor Author

#29258

This was referenced Mar 16, 2021
fabOnReact added a commit to fabOnReact/react-native that referenced this pull request Mar 23, 2022
Following same implementation used with underlineColorAndroid
https://github.com/fabriziobertoglio1987/react-native/blob/15810e96d90e18dbd424666338fdec0127d403ed/Libraries/Components/TextInput/TextInput.js#L470
https://github.com/fabriziobertoglio1987/react-native/blob/15810e96d90e18dbd424666338fdec0127d403ed/Libraries/Components/TextInput/AndroidTextInputNativeComponent.js#L203

While with other components (for example prop android_ripple), we use
the android_ prefix to denote platform specific props.
facebook#29157 (comment)
https://github.com/fabriziobertoglio1987/react-native/blob/15810e96d90e18dbd424666338fdec0127d403ed/Libraries/Components/Pressable/Pressable.js#L177

In the case of TextInput we already have Platform Logic that detects
Android/iOS platform.
https://github.com/fabriziobertoglio1987/react-native/blob/15810e96d90e18dbd424666338fdec0127d403ed/Libraries/Components/TextInput/TextInput.js#L1268

For this reason TextInput component does not use android_ props and
instead uses this naming convention underlineColorAndroid.

To be noted that the prop underlineColorAndroid is passed to both iOS
and Android version, while other props have platform specific logic for
android and iOS.
https://github.com/fabriziobertoglio1987/react-native/blob/15810e96d90e18dbd424666338fdec0127d403ed/Libraries/Components/TextInput/TextInput.js#L1334
https://github.com/fabriziobertoglio1987/react-native/blob/15810e96d90e18dbd424666338fdec0127d403ed/Libraries/Components/TextInput/TextInput.js#L1293

Example of a prop that have a specific value on Android and is different
from iOS
https://github.com/fabriziobertoglio1987/react-native/blob/15810e96d90e18dbd424666338fdec0127d403ed/Libraries/Components/TextInput/TextInput.js#L1271

I decided to follow the same solution used in underlineColorAndroid.
fabOnReact added a commit to fabOnReact/react-native that referenced this pull request Mar 23, 2022
Following same implementation used with underlineColorAndroid
https://github.com/fabriziobertoglio1987/react-native/blob/15810e96d90e18dbd424666338fdec0127d403ed/Libraries/Components/TextInput/TextInput.js#L470
https://github.com/fabriziobertoglio1987/react-native/blob/15810e96d90e18dbd424666338fdec0127d403ed/Libraries/Components/TextInput/AndroidTextInputNativeComponent.js#L203

While with other components (for example prop android_ripple), we use
the android_ prefix to denote platform specific props.
facebook#29157 (comment)
https://github.com/fabriziobertoglio1987/react-native/blob/15810e96d90e18dbd424666338fdec0127d403ed/Libraries/Components/Pressable/Pressable.js#L177

In the case of TextInput we already have Platform Logic that detects
Android/iOS platform.
https://github.com/fabriziobertoglio1987/react-native/blob/15810e96d90e18dbd424666338fdec0127d403ed/Libraries/Components/TextInput/TextInput.js#L1268

For this reason TextInput component does not use android_ props and
instead uses this naming convention underlineColorAndroid.

To be noted that the prop underlineColorAndroid is passed to both iOS
and Android version, while other props have platform specific logic for
android and iOS.
https://github.com/fabriziobertoglio1987/react-native/blob/15810e96d90e18dbd424666338fdec0127d403ed/Libraries/Components/TextInput/TextInput.js#L1334
https://github.com/fabriziobertoglio1987/react-native/blob/15810e96d90e18dbd424666338fdec0127d403ed/Libraries/Components/TextInput/TextInput.js#L1293

Example of a prop that have a specific value on Android and is different
from iOS
https://github.com/fabriziobertoglio1987/react-native/blob/15810e96d90e18dbd424666338fdec0127d403ed/Libraries/Components/TextInput/TextInput.js#L1271

I decided to follow the same solution used in underlineColorAndroid.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Merged This PR has been merged. Platform: Android Android applications.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Android ignores unicode soft hyphen (u00AD)
6 participants