-
Notifications
You must be signed in to change notification settings - Fork 24.3k
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 textTransform style support #18387
Conversation
# Conflicts: # Libraries/StyleSheet/StyleSheetTypes.js # RNTester/js/TextExample.ios.js # React/React.xcodeproj/project.pbxproj
I see that 2 checks failed. I'm not clear what my responsibility is here, as they appear (to me!) to be failing for reasons unrelated to my changes. Someone please advise?! |
neat! |
@@ -203,6 +203,7 @@ export type TextStyle = $ReadOnly<{| | |||
| 'underline line-through', | |||
textDecorationStyle?: 'solid' | 'double' | 'dotted' | 'dashed', | |||
textDecorationColor?: ColorValue, | |||
+textTransform?: 'none' | 'capitalize' | 'uppercase' | 'lowercase', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You don't need the +
since the whole object is wrapped with $ReadOnly
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please also fix code style?
Thank you!
Libraries/Text/RCTTextAttributes.m
Outdated
@@ -214,6 +217,22 @@ - (UIColor *)effectiveBackgroundColor | |||
return effectiveBackgroundColor ?: [UIColor clearColor]; | |||
} | |||
|
|||
- (NSString*) effectiveText: (NSString*) text |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please rename it to applyTextAttributesToText:
and follow code style.
Libraries/Text/RCTTextAttributes.m
Outdated
@@ -73,6 +73,9 @@ - (void)applyTextAttributes:(RCTTextAttributes *)textAttributes | |||
_isHighlighted = textAttributes->_isHighlighted || _isHighlighted; // * | |||
_tag = textAttributes->_tag ?: _tag; | |||
_layoutDirection = textAttributes->_layoutDirection != UIUserInterfaceLayoutDirectionLeftToRight ? textAttributes->_layoutDirection : _layoutDirection; | |||
|
|||
// Transform | |||
_textTransform = textAttributes->_textTransform ?: _textTransform; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Transform
can be considered as Special
, so remove the comment.
Compare to additional special "undefined" value here.
Libraries/Text/RCTTextAttributes.m
Outdated
{ | ||
switch (_textTransform) | ||
{ | ||
case RCTTextTransformNone: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add additional "undefined" value.
React/Views/RCTTextTransform.h
Outdated
#import <Foundation/Foundation.h> | ||
|
||
typedef NS_ENUM(NSInteger, RCTTextTransform) { | ||
RCTTextTransformNone = 0, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add additional "undefined" value.
Libraries/Text/RCTTextAttributes.h
Outdated
/** | ||
* Text transformed per 'none', 'uppercase', 'lowercase', 'capitalize' | ||
*/ | ||
- (NSString*) effectiveText: (NSString*) text; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Fix spaces here to match the project code style - (NSString *)effectiveText:(NSString *)text;
@TomSwift Nice work! CI is failing on master so it doesn't look related to your PR. |
@shergin Why do we need an undefined value here? AFAIK the prop will default to RCTTextTransformNone when not specified because of the RCTConvert macro here https://github.com/facebook/react-native/pull/18387/files#diff-99f3601ec9fb0cc23570af725a27781eR324. |
@"capitalize": @(RCTTextTransformCapitalize), | ||
@"uppercase": @(RCTTextTransformUppercase), | ||
@"lowercase": @(RCTTextTransformLowercase), | ||
}), RCTTextTransformNone, integerValue) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The default value here should be RCTTextTransformUndefined
, not RCTTextTransformNone
.
@janicduplessis Right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm not how this works, does [RCTTextAttributes applyTextAttributes] only receives changed attributes (and some undefined value for the ones that did not change)?
I think both options would work but I'm not sure if one makes more sense. Maybe check what we do for other enum text attributes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's easy to test.
<Text uppercase>a<Text>b</Text>c</Text>
must produce "ABC", but with RCTTextTransformNone
as a default value it will produce "AbC", I believe.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@janicduplessis Yeah, seems we have to specify |
Overall it looks good to me! |
@@ -73,6 +74,7 @@ - (void)applyTextAttributes:(RCTTextAttributes *)textAttributes | |||
_isHighlighted = textAttributes->_isHighlighted || _isHighlighted; // * | |||
_tag = textAttributes->_tag ?: _tag; | |||
_layoutDirection = textAttributes->_layoutDirection != UIUserInterfaceLayoutDirectionLeftToRight ? textAttributes->_layoutDirection : _layoutDirection; | |||
_textTransform = textAttributes->_textTransform != RCTTextTransformUndefined ? textAttributes->_textTransform : _textTransform; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, this is why we have to have undefined
value!
It seems like this would be great to get screenshot tests for |
@TheSavior Not sure if we still run those tests anywhere but there is one already for TextExample.js so we probably just need to update the screenshot and this would be tested already! |
@shergin - is this good to go, then? It's still showing "Changes requested" but I'm not clear that there's anything else for me to address. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Had another look, looks good just a few nits.
I'll let @shergin have another look too and ship it
Libraries/Text/RCTTextAttributes.m
Outdated
switch (_textTransform) { | ||
case RCTTextTransformUndefined: | ||
case RCTTextTransformNone: | ||
default: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we need a default case since we cover all enum values.
Libraries/Text/RCTTextAttributes.m
Outdated
@@ -214,6 +216,22 @@ - (UIColor *)effectiveBackgroundColor | |||
return effectiveBackgroundColor ?: [UIColor clearColor]; | |||
} | |||
|
|||
- (NSString*)applyTextAttributesToText:(NSString*)text |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Space between type and pointer (NSString *) for both
Sorry for style nits, need prettier for objc!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the last round of feedback I was asked to fix the formatting to what it is now, which matches the formatting in the surrounding code (no spaces). My personal preference is spaces, but I think my additions should match the surrounding code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There should be a space between NSString and *, the rest is fine
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
gotcha; fixed.
Libraries/Text/RCTTextAttributes.h
Outdated
/** | ||
* Text transformed per 'none', 'uppercase', 'lowercase', 'capitalize' | ||
*/ | ||
- (NSString*)applyTextAttributesToText:(NSString*)text; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same, spaces
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same; matches surrounding code.
@@ -51,5 +51,7 @@ - (RCTShadowView *)shadowView | |||
RCT_REMAP_SHADOW_PROPERTY(textShadowColor, textAttributes.textShadowColor, UIColor) | |||
// Special | |||
RCT_REMAP_SHADOW_PROPERTY(isHighlighted, textAttributes.isHighlighted, BOOL) | |||
// Transform |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove this comment, textTransform can be considered in the "Special" section
Libraries/Text/RCTTextAttributes.m
Outdated
@@ -263,7 +281,9 @@ - (BOOL)isEqual:(RCTTextAttributes *)textAttributes | |||
// Special | |||
RCTTextAttributesCompareOthers(_isHighlighted) && | |||
RCTTextAttributesCompareObjects(_tag) && | |||
RCTTextAttributesCompareOthers(_layoutDirection); | |||
RCTTextAttributesCompareOthers(_layoutDirection) && | |||
// Transform |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same, remove comment
@shergin Are you still looking to see changes applied here? |
I am wondering whether we should also provide Android support - otherwise, I am afraid this may end up in an error on Android. |
It should just be a no-op on Android for now, adding support for it in a follow up PR would be great. |
I think @shergin is very busy :) so I’ll just go ahead and ship it, looks good to me. @facebook-github-bot shipit |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@janicduplessis is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
render: function() { | ||
return ( | ||
<View> | ||
<Text style={{ textTransform: 'uppercase'}}> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should add a test with <Text>
with no textTransform
style.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@janicduplessis Thanks!!1 |
@facebook-github-bot shipit |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@janicduplessis is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
Summary: Issue [facebook#2088](facebook#2088). The basic desire is to have a declarative mechanism to transform text content to uppercase or lowercase or titlecase ("capitalized"). My test plan involves having added a test-case to the RNTester app within the `<Text>` component area. I then manually verified that the rendered content met my expectation. Here is the markup that exercises my enhancement: ``` <View> <Text style={{ textTransform: 'uppercase'}}> This text should be uppercased. </Text> <Text style={{ textTransform: 'lowercase'}}> This TEXT SHOULD be lowercased. </Text> <Text style={{ textTransform: 'capitalize'}}> This text should be CAPITALIZED. </Text> <Text style={{ textTransform: 'capitalize'}}> Mixed:{' '} <Text style={{ textTransform: 'uppercase'}}> uppercase{' '} </Text> <Text style={{ textTransform: 'lowercase'}}> LoWeRcAsE{' '} </Text> <Text style={{ textTransform: 'capitalize'}}> capitalize each word </Text> </Text> </View> ``` And here is a screenshot of the result: ![screen shot 2018-03-14 at 3 01 02 pm](https://user-images.githubusercontent.com/575821/37433772-7abe7fa0-279a-11e8-9ec9-fb3aa1952dad.png) [Website Documentation PR](facebook/react-native-website#254) facebook/react-native-website#254 [IOS] [ENHANCEMENT] [Text] - added textTransform style property enabling declarative casing transformations Closes facebook#18387 Differential Revision: D7583315 Pulled By: shergin fbshipit-source-id: a5d22aea2aa4f494b7b25a055abe64799ccbaa79
Summary: Issue [facebook#2088](facebook#2088). The basic desire is to have a declarative mechanism to transform text content to uppercase or lowercase or titlecase ("capitalized"). My test plan involves having added a test-case to the RNTester app within the `<Text>` component area. I then manually verified that the rendered content met my expectation. Here is the markup that exercises my enhancement: ``` <View> <Text style={{ textTransform: 'uppercase'}}> This text should be uppercased. </Text> <Text style={{ textTransform: 'lowercase'}}> This TEXT SHOULD be lowercased. </Text> <Text style={{ textTransform: 'capitalize'}}> This text should be CAPITALIZED. </Text> <Text style={{ textTransform: 'capitalize'}}> Mixed:{' '} <Text style={{ textTransform: 'uppercase'}}> uppercase{' '} </Text> <Text style={{ textTransform: 'lowercase'}}> LoWeRcAsE{' '} </Text> <Text style={{ textTransform: 'capitalize'}}> capitalize each word </Text> </Text> </View> ``` And here is a screenshot of the result: ![screen shot 2018-03-14 at 3 01 02 pm](https://user-images.githubusercontent.com/575821/37433772-7abe7fa0-279a-11e8-9ec9-fb3aa1952dad.png) [Website Documentation PR](facebook/react-native-website#254) facebook/react-native-website#254 [IOS] [ENHANCEMENT] [Text] - added textTransform style property enabling declarative casing transformations Closes facebook#18387 Differential Revision: D7583315 Pulled By: shergin fbshipit-source-id: a5d22aea2aa4f494b7b25a055abe64799ccbaa79
Summary: Issue #2088 (closed, but a bit pre-emptively imo, since Android support was skipped) Related (merged) iOS PR #18387 Related documentation PR facebook/react-native-website#500 The basic desire is to have a declarative mechanism to transform text content to uppercase or lowercase or titlecase ("capitalized"). Pull Request resolved: #20572 Differential Revision: D9311716 Pulled By: hramos fbshipit-source-id: dfbb855117196958e7ae5e980700d31be07a448d
Summary: Issue [#2088](facebook/react-native#2088). The basic desire is to have a declarative mechanism to transform text content to uppercase or lowercase or titlecase ("capitalized"). My test plan involves having added a test-case to the RNTester app within the `<Text>` component area. I then manually verified that the rendered content met my expectation. Here is the markup that exercises my enhancement: ``` <View> <Text style={{ textTransform: 'uppercase'}}> This text should be uppercased. </Text> <Text style={{ textTransform: 'lowercase'}}> This TEXT SHOULD be lowercased. </Text> <Text style={{ textTransform: 'capitalize'}}> This text should be CAPITALIZED. </Text> <Text style={{ textTransform: 'capitalize'}}> Mixed:{' '} <Text style={{ textTransform: 'uppercase'}}> uppercase{' '} </Text> <Text style={{ textTransform: 'lowercase'}}> LoWeRcAsE{' '} </Text> <Text style={{ textTransform: 'capitalize'}}> capitalize each word </Text> </Text> </View> ``` And here is a screenshot of the result: ![screen shot 2018-03-14 at 3 01 02 pm](https://user-images.githubusercontent.com/575821/37433772-7abe7fa0-279a-11e8-9ec9-fb3aa1952dad.png) [Website Documentation PR](facebook/react-native-website#254) facebook/react-native-website#254 [IOS] [ENHANCEMENT] [Text] - added textTransform style property enabling declarative casing transformations Closes facebook/react-native#18387 Differential Revision: D7583315 Pulled By: shergin fbshipit-source-id: a5d22aea2aa4f494b7b25a055abe64799ccbaa79
Summary: Issue #2088 (closed, but a bit pre-emptively imo, since Android support was skipped) Related (merged) iOS PR #18387 Related documentation PR facebook/react-native-website#500 The basic desire is to have a declarative mechanism to transform text content to uppercase or lowercase or titlecase ("capitalized"). Pull Request resolved: #20572 Differential Revision: D9311716 Pulled By: hramos fbshipit-source-id: dfbb855117196958e7ae5e980700d31be07a448d
Summary: Issue facebook/react-native#2088 (closed, but a bit pre-emptively imo, since Android support was skipped) Related (merged) iOS PR facebook/react-native#18387 Related documentation PR facebook/react-native-website#500 The basic desire is to have a declarative mechanism to transform text content to uppercase or lowercase or titlecase ("capitalized"). Pull Request resolved: facebook/react-native#20572 Differential Revision: D9311716 Pulled By: hramos fbshipit-source-id: dfbb855117196958e7ae5e980700d31be07a448d
Motivation
Issue #2088.
The basic desire is to have a declarative mechanism to transform text content to uppercase or lowercase or titlecase ("capitalized").
Test Plan
My test plan involves having added a test-case to the RNTester app within the
<Text>
component area. I then manually verified that the rendered content met my expectation.Here is the markup that exercises my enhancement:
And here is a screenshot of the result:
Related PRs
Website Documentation PR
facebook/react-native-website#254
Release Notes
[IOS] [ENHANCEMENT] [Text] - added textTransform style property enabling declarative casing transformations