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

[Text] Add textDecoration style attributes #845

Closed
wants to merge 6 commits into from

Conversation

KJlmfe
Copy link
Contributor

@KJlmfe KJlmfe commented Apr 14, 2015

I add textDecorationLine, textDecorationStyle, textDecorationColor style property for Text module.

And it follows the CSS naming convention and using method is same with CSS.

  1. textDecorationLine refers to: https://developer.mozilla.org/en-US/docs/Web/CSS/text-decoration-line
  2. textDecorationStyle refers to: https://developer.mozilla.org/en-US/docs/Web/CSS/text-decoration-style
  3. textDecorationColorrefers to: https://developer.mozilla.org/en-US/docs/Web/CSS/text-decoration-color

Here is a simple demo:

<Text style={{textDecorationLine: 'underline', textDecorationStyle: 'solid'}}>
  Solid underline
</Text>
<Text style={{textDecorationLine: 'underline', textDecorationStyle: 'double', textDecorationColor: '#ff0000'}}>
  Double underline with custom color
</Text>
<Text style={{textDecorationLine: 'underline', textDecorationStyle: 'dashed', textDecorationColor: '#9CDC40'}}>
  Dashed underline with custom color
</Text>
<Text style={{textDecorationLine: 'underline', textDecorationStyle: 'dotted', textDecorationColor: 'blue'}}>
  Dotted underline with custom color
</Text>
<Text style={{textDecorationLine: 'none'}}>
  None textDecoration
</Text>
<Text style={{textDecorationLine: 'line-through', textDecorationStyle: 'solid'}}>
  Solid line-through
</Text>
<Text style={{textDecorationLine: 'line-through', textDecorationStyle: 'double', textDecorationColor: '#ff0000'}}>
  Double line-through with custom color
</Text>
<Text style={{textDecorationLine: 'line-through', textDecorationStyle: 'dashed', textDecorationColor: '#9CDC40'}}>
  Dashed line-through with custom color
</Text>
<Text style={{textDecorationLine: 'line-through', textDecorationStyle: 'dotted', textDecorationColor: 'blue'}}>
  Dotted line-through with custom color
</Text>
<Text style={{textDecorationLine: 'underline line-through'}}>
  Both underline and line-through
</Text>


2015-04-15 8 48 20

@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 Apr 14, 2015
@vjeux
Copy link
Contributor

vjeux commented Apr 14, 2015

Thanks for the pull request! Would be nice to follow the CSS naming convention for this
https://developer.mozilla.org/en-US/docs/Web/CSS/text-decoration-style

And, if you can provide a jsfiddle that shows that the code works on a span on the web that would be awesome!

@KJlmfe
Copy link
Contributor Author

KJlmfe commented Apr 15, 2015

@vjeux I changed the property name to follow the text-decoration-style, such as "solid", "double", "dashed", "dotted".

And, text-decoration-style is only support on firefox. So the jsfiddle web demo is not really needed.

@vjeux
Copy link
Contributor

vjeux commented Apr 15, 2015

We're trying to use web naming whenever possible, even if it's not implemented everywhere. This way we can redirect users to mdn documentation, and don't have to bikeshed on naming.

@bakso
Copy link

bakso commented Apr 15, 2015

That's awesome!
I desire this feature for a long time!

@KJlmfe KJlmfe changed the title <Text> module style add strikeThrough and strikeThroughColor Add "textDecorationLine, textDecorationStyle, textDecorationColor" style property for <Text> module Apr 15, 2015
@KJlmfe KJlmfe changed the title Add "textDecorationLine, textDecorationStyle, textDecorationColor" style property for <Text> module Fix https://github.com/facebook/react-native/issues/753 Add "textDecorationLine, textDecorationStyle, textDecorationColor" style property for <Text> module Apr 15, 2015
@KJlmfe KJlmfe changed the title Fix https://github.com/facebook/react-native/issues/753 Add "textDecorationLine, textDecorationStyle, textDecorationColor" style property for <Text> module Fix #753 , Add "textDecorationLine, textDecorationStyle, textDecorationColor" style property for <Text> module Apr 15, 2015
@KJlmfe
Copy link
Contributor Author

KJlmfe commented Apr 15, 2015

@vjeux I rewrite the code and change the pull request title and content. Now it follows the CSS naming convention and using method is same with CSS.

And here is working on web demo: https://jsfiddle.net/kjlmfe/a1zvL6s8/1/ (Only for firefox)

@luics
Copy link

luics commented Apr 16, 2015

cool

1 similar comment
@mariodu
Copy link

mariodu commented Apr 16, 2015

cool

@liangfeidotme
Copy link

awesome ! DiaoZhaTian

@SoXeon
Copy link

SoXeon commented Apr 17, 2015

66666!

@imochen
Copy link

imochen commented Apr 17, 2015

看到,吊炸天。。。

@brentvatne
Copy link
Collaborator

@sahrens - looked at the git blame and wasn't quite sure who to assign here, your name stood out so I just added you, re-assign if I'm off.

@KJlmfe - thanks! can we get a rebase?

@brentvatne brentvatne changed the title Fix #753 , Add "textDecorationLine, textDecorationStyle, textDecorationColor" style property for <Text> module [Text] Add textDecoration style attributes Jun 1, 2015
@sahrens
Copy link
Contributor

sahrens commented Jun 1, 2015

Looks great! Can you rebase, squash your commits, and re-record the TextExample snapshot test? Set reRecord yes here and run with cmd+U:

https://github.com/facebook/react-native/blob/master/Examples/UIExplorer/UIExplorerTests/UIExplorerTests.m#L97

Also, assigning over to @a2 who is our Text master.

@cssoul
Copy link

cssoul commented Jun 2, 2015

well done~

@KJlmfe
Copy link
Contributor Author

KJlmfe commented Jun 3, 2015

@sahrens I had merge branch 'facebook/react-native/master' into my master and resolve the conflicts.

@brentvatne
Copy link
Collaborator

@KJlmfe - instead of merging could you do a git rebase -i to squash the commits into one? See this guide

@brentvatne
Copy link
Collaborator

@KJlmfe - I'm confused about what's going on here, I will close this but will happily re-open if you rebase and squash into one commit! Thanks 😄

@brentvatne brentvatne closed this Jun 23, 2015
@KJlmfe
Copy link
Contributor Author

KJlmfe commented Jun 29, 2015

@brentvatne I had rebased and squashed into one commit!

See it KJlmfe@fdaca5d

@pcottle
Copy link
Contributor

pcottle commented Jun 30, 2015

Can we reopen this? Would be sweet to get underlining in :D

@MossP
Copy link

MossP commented Jul 3, 2015

did this ever get merged?

@pcottle
Copy link
Contributor

pcottle commented Jul 4, 2015

I threw up a fresher version of this over at #1869 that was rebased, hopefully once the Travis CI build is green we can merge!

sahrens pushed a commit to sahrens/react-native that referenced this pull request Jul 7, 2015
Summary:
This is simply a rebased and squashed version of @KJlmfe's PR over at facebook#845

It was actually already squashed into one commit, but for some reason that was hard to see from the original PR.
Closes facebook#1869
Github Author: KJlmfe <kjlmfe@gmail.com>
@pcottle
Copy link
Contributor

pcottle commented Jul 11, 2015

Woohoo! this made it into the v0.8.0-rc release:
https://github.com/facebook/react-native/releases/tag/v0.8.0-rc

underline all the things

@MossP
Copy link

MossP commented Jul 11, 2015

👍

@MossP
Copy link

MossP commented Jul 14, 2015

Does it work for you, @pcottle? I don't seem to be seeing any text decoration.

@ide
Copy link
Contributor

ide commented Jul 14, 2015

It's working in the UIExplorer.

@MossP
Copy link

MossP commented Jul 14, 2015

Ah, it seems that my project has reverted to 0.7.1 somewhere along the line. Apologies. I'll go try again.

@MossP
Copy link

MossP commented Jul 14, 2015

That fixed it, thanks @ide. Sorry for the newbie error.

aleclarson pushed a commit to aleclarson/react-native that referenced this pull request Sep 20, 2015
Summary:
This is simply a rebased and squashed version of @KJlmfe's PR over at facebook#845

It was actually already squashed into one commit, but for some reason that was hard to see from the original PR.
Closes facebook#1869
Github Author: KJlmfe <kjlmfe@gmail.com>
@herbertdai
Copy link

Does it support Android in 0.19.0?

@pewh
Copy link

pewh commented Jun 6, 2017

Hello, what's the current status of this? I need textDecorationColor props & it didn't work on RN 0.44

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.