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.

Sign up for GitHub

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

Jump to bottom

Added support for textDecorationLine style prop on Android #5105

Closed
wants to merge 1 commit into from
Closed

Added support for textDecorationLine style prop on Android #5105

wants to merge 1 commit into from

Conversation

Charca
Copy link
Contributor

@Charca Charca commented Jan 4, 2016

As suggested by @kmagiera in #3819, I've implemented textDecorationLine style for Android in ReactTextShadowNode using span operations so we can support nested Text components.

Demo

Demo

@facebook-github-bot
Copy link
Contributor

By analyzing the blame information on this pull request, we identified @mkonicek, @kmagiera and @dmmiller 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 Jan 4, 2016
kmagiera
kmagiera reviewed Jan 4, 2016
View reviewed changes
ReactAndroid/src/main/java/com/facebook/react/views/text/ReactTextShadowNode.java
mIsUnderlineTextDecorationSet = (
"underline".equals(textDecorationLineString) ||
"underline line-through".equals(textDecorationLineString));
mIsLineThroughTextDecorationSet = (
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this a conscious decision or some sort of standard to only support "underline line-though" but not "line-through underline"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was just to be consistent with the iOS version.

@facebook-github-bot
Copy link
Contributor

@Charca updated the pull request.

@Charca
Copy link
Contributor Author

Charca commented Jan 27, 2016

@kmagiera latest update lets us support bot "underline line-through" and "line-through underline"

@mkonicek
Copy link
Contributor

Oh cool! Should we close #3819?

@Charca can you rebase please?

@andreicoman11 Can you review?

@facebook-github-bot
Copy link
Contributor

@Charca updated the pull request.

@Charca
Copy link
Contributor Author

Charca commented Mar 20, 2016

@mkonicek Done! I'm not up to date with the discussion on #3819, but if the problem over there is the support for nested Text components, I guess you can safely close it.

@mkonicek
Copy link
Contributor

👍 Thanks @Charca.

@ghost
Copy link

ghost commented Mar 27, 2016

@mkonicek would you mind taking a look at this pull request? It's been a while since the last commit was reviewed.

@terrysahaidak
Copy link

What's about dashed/dotted underline?

@mkonicek
Copy link
Contributor

mkonicek commented Apr 1, 2016

I'll let @kmagiera or @andreicoman11 review this one.

andreicoman11
andreicoman11 reviewed Apr 5, 2016
View reviewed changes
ReactAndroid/src/main/java/com/facebook/react/views/text/ReactTextShadowNode.java
@@ -428,6 +439,18 @@ public void setFontStyle(@Nullable String fontStyleString) {
}
}

@ReactProp(name = ViewProps.TEXT_DECORATION_LINE)
public void setTextDecorationLine(@Nullable String textDecorationLineString) {
for(String textDecorationLineSubString:textDecorationLineString.split(" ")) {
Copy link
Contributor

Choose a reason for hiding this comment

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

what if textDecorationLineString is null?

@facebook-github-bot
Copy link
Contributor

@Charca updated the pull request.

@ghost
Copy link

ghost commented Apr 10, 2016

@mkonicek would you mind taking a look at this pull request? It's been a while since the last commit was reviewed.

andreicoman11
andreicoman11 reviewed Apr 11, 2016
View reviewed changes
ReactAndroid/src/test/java/com/facebook/react/views/text/ReactTextTest.java

UnderlineSpan underlineSpan =
getSingleSpan((TextView) rootView.getChildAt(0), UnderlineSpan.class);
assertThat(underlineSpan instanceof UnderlineSpan).isTrue();
Copy link
Contributor

Choose a reason for hiding this comment

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

assert other Span is false

@facebook-github-bot
Copy link
Contributor

@Charca updated the pull request.

@andreicoman11
Copy link
Contributor

Looks good! Thank you!

@andreicoman11
Copy link
Contributor

@facebook-github-bot shipit

@facebook-github-bot
Copy link
Contributor

Thanks for importing. If you are an FB employee go to Phabricator to review.

@ghost ghost closed this in 2039be9 Apr 12, 2016
zebulgar pushed a commit to nightingale/react-native that referenced this pull request Jun 18, 2016
@Charca @zebulgar
Added support for textDecorationLine style prop on Android
33e7396 
Summary:As suggested by kmagiera in facebook#3819, I've implemented `textDecorationLine` style for Android in `ReactTextShadowNode` using span operations so we can support nested Text components.

![Demo](http://c.hlp.sc/022k2l033p0n/Image%202016-01-03%20at%2011.17.15%20PM.png)
Closes facebook#5105

Differential Revision: D3167756

Pulled By: andreicoman11

fb-gh-sync-id: 122701a53d50f47f89b49e1f343c97db5fa2323d
fbshipit-source-id: 122701a53d50f47f89b49e1f343c97db5fa2323d
@AndreiCalazans AndreiCalazans mentioned this pull request May 16, 2018
Closed
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@andreicoman11 andreicoman11

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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@Charca @facebook-github-bot @mkonicek @terrysahaidak @andreicoman11 @kmagiera