-
Notifications
You must be signed in to change notification settings - Fork 819
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
Add support for textDecorationLine property #143
Conversation
bd756a6
to
8bc9803
Compare
Wooo, great work @davidseik :)
Will dig in this afternon |
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.
Looks great, other than the one property name I flagged.
(btw I'm on vacation next week - feel free to ping other maintainers to get this merged if I miss it)
@@ -12,13 +12,14 @@ const INHERITABLE_STYLES = [ | |||
'fontSize', | |||
'fontStyle', | |||
'fontWeight', | |||
'textAlign', | |||
'textDecoration', |
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 mirror the React Native APIs as much as possible — can you change this to textDecorationLine
to match?
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.
Hi Jon, I actually wanted to use the exact same API of textDecorationLine but I found this:
https://github.com/airbnb/react-sketchapp/blob/master/src/stylesheet/expandStyle.js#L61
When the style gets passed down as a textStyle this part expands it to be renamed textDecoration
; I don't know why this is needed. I can go ahead and remove the shortHand here and add the textDecorationLine
over here so it's propagated like it. Thoughts?
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 think maybe it was actually correct the first time - looks like I have your original commit and it's indeed working properly by expanding the textDecorationLine into textDecoration. Not sure what went wrong the first time I reviewed it, sorry!
Documentation update looks good - we want the public interface to be textDecorationLine
.
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.
Got it, then let me change it back.
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.
Funny thing is that because of the expansion they both work textDecoration
& textDecorationLine
; Thoughts?
34e888b
to
2775364
Compare
2775364
to
7a215c3
Compare
@lelandrichardson @BenjaminCorey can you guys check if this can be merged? |
Hi, this adds support for the textDecorationLine property for text.
Tested on Sketch 44.1
Example of usage:
which outputs this:
The API supports the following attributes:
none
,underline
,line-through
,double
.I have some questions though: