[Text] Add Drop Shadow Style to Text Component #2410

Closed
wants to merge 2 commits into
from

Conversation

Projects
None yet
7 participants
@greghe
Contributor

greghe commented Aug 23, 2015

dropshadow

Adds four new styles to the Text component: textShadowOpacity,textShadowColor, textShadowOffset and textShadowRadius. They are nearly identical to the correspondingly named view shadow properties.

Adds four new styles to text: textShadowOpacity, textShadowColor, tex…
…tShadowOffset and textShadowRadius. Their behavior is similar to the view counterparts.
@ide

This comment has been minimized.

Show comment
Hide comment
@ide

ide Aug 23, 2015

Collaborator

LGTM. I'm wondering if we should drop the textShadowOpacity/shadowOpacity prop since the shadowColor should take an rgba value anyway. @vjeux do you agree with that or know why shadow opacity and color were separate props?

Collaborator

ide commented Aug 23, 2015

LGTM. I'm wondering if we should drop the textShadowOpacity/shadowOpacity prop since the shadowColor should take an rgba value anyway. @vjeux do you agree with that or know why shadow opacity and color were separate props?

@ide

View changes

Libraries/Text/RCTShadowText.m
@@ -201,6 +202,22 @@ - (NSAttributedString *)_attributedStringWithFontFamily:(NSString *)fontFamily
if (_color) {
[self _addAttribute:NSForegroundColorAttributeName withValue:_color toAttributedString:attributedString];
}
+
+ if (!isnan(_textShadowOpacity)) {
+ NSShadow *shadow = [[NSShadow alloc] init];

This comment has been minimized.

@ide

ide Aug 23, 2015

Collaborator

(new coding guideline) [NSShadow new]

@ide

ide Aug 23, 2015

Collaborator

(new coding guideline) [NSShadow new]

@ide

View changes

Libraries/Text/RCTShadowText.m
@@ -180,7 +181,7 @@ - (NSAttributedString *)_attributedStringWithFontFamily:(NSString *)fontFamily
if (!isnan(_letterSpacing)) {
letterSpacing = @(_letterSpacing);
}
-
+

This comment has been minimized.

@ide

ide Aug 23, 2015

Collaborator

remove

@ide

ide Aug 23, 2015

Collaborator

remove

@greghe

This comment has been minimized.

Show comment
Hide comment
@greghe

greghe Aug 23, 2015

Contributor

I think there should be separate opacity and color styles for text shadows, just as there are separate shadowColor and shadowOpacity styles for views. For one, it makes it easy for the user (and the software) to toggle the shadows on and off completely.

Contributor

greghe commented Aug 23, 2015

I think there should be separate opacity and color styles for text shadows, just as there are separate shadowColor and shadowOpacity styles for views. For one, it makes it easy for the user (and the software) to toggle the shadows on and off completely.

@ide

This comment has been minimized.

Show comment
Hide comment
@ide

ide Aug 24, 2015

Collaborator

I'm proposing to consolidate the props for Views too. You could just as easily specify shadowColor: 'transparent' to quickly toggle the shadows.

Collaborator

ide commented Aug 24, 2015

I'm proposing to consolidate the props for Views too. You could just as easily specify shadowColor: 'transparent' to quickly toggle the shadows.

@vjeux

This comment has been minimized.

Show comment
Hide comment
@vjeux

vjeux Aug 27, 2015

Contributor

Would be nice to follow the CSS-spec naming convention in case of conflicts.

https://developer.mozilla.org/en-US/docs/Web/CSS/box-shadow
https://developer.mozilla.org/en-US/docs/Web/CSS/text-shadow

In this case, yeah, I agree that having an rgba color would be great

Contributor

vjeux commented Aug 27, 2015

Would be nice to follow the CSS-spec naming convention in case of conflicts.

https://developer.mozilla.org/en-US/docs/Web/CSS/box-shadow
https://developer.mozilla.org/en-US/docs/Web/CSS/text-shadow

In this case, yeah, I agree that having an rgba color would be great

@ide

This comment has been minimized.

Show comment
Hide comment
@ide

ide Aug 27, 2015

Collaborator

@vjeux for the shadow properties, CSS only has the shorthand forms where you have to specify offset, radius, and color in one string together but I don't think we want that. I do think it would be good to support multiple shadows so maybe the syntax we want looks like this:

textShadow: { offset, radius, color }
// and also
textShadow: [
  { offset1, radius1, color1 },
  { offset2, radius2, color2 }
]
Collaborator

ide commented Aug 27, 2015

@vjeux for the shadow properties, CSS only has the shorthand forms where you have to specify offset, radius, and color in one string together but I don't think we want that. I do think it would be good to support multiple shadows so maybe the syntax we want looks like this:

textShadow: { offset, radius, color }
// and also
textShadow: [
  { offset1, radius1, color1 },
  { offset2, radius2, color2 }
]
@vjeux

This comment has been minimized.

Show comment
Hide comment
@vjeux

vjeux Aug 27, 2015

Contributor

sounds good with me :)

Contributor

vjeux commented Aug 27, 2015

sounds good with me :)

@ide

This comment has been minimized.

Show comment
Hide comment
@ide

ide Aug 27, 2015

Collaborator

@greghe how does the proposal in my comment above sound? Initially we can support just one shadow instead of several ones, but I would like to start with the API we want so that we don't have to deprecate an old one or support several ways to do the same thing.

Collaborator

ide commented Aug 27, 2015

@greghe how does the proposal in my comment above sound? Initially we can support just one shadow instead of several ones, but I would like to start with the API we want so that we don't have to deprecate an old one or support several ways to do the same thing.

@greghe

This comment has been minimized.

Show comment
Hide comment
@greghe

greghe Aug 27, 2015

Contributor

Sure, that sounds fine.

Contributor

greghe commented Aug 27, 2015

Sure, that sounds fine.

@jim-lake

This comment has been minimized.

Show comment
Hide comment
@jim-lake

jim-lake Aug 30, 2015

Some form of this would be very useful, text shadows are used heavily in native apps.

Some form of this would be very useful, text shadows are used heavily in native apps.

@ide

This comment has been minimized.

Show comment
Hide comment
@ide

ide Sep 2, 2015

Collaborator

@greghe can you update this PR to implement the textShadow: { offset, radius, color } API? Feel free to ping me if the API is unclear.

Collaborator

ide commented Sep 2, 2015

@greghe can you update this PR to implement the textShadow: { offset, radius, color } API? Feel free to ping me if the API is unclear.

@ide ide changed the title from Add Drop Shadow Style to Text Component to [Text] Add Drop Shadow Style to Text Component Sep 2, 2015

@mkonicek

This comment has been minimized.

Show comment
Hide comment
@mkonicek

mkonicek Nov 11, 2015

Contributor

How is this going to work on Android? Can it have the same API?

Contributor

mkonicek commented Nov 11, 2015

How is this going to work on Android? Can it have the same API?

@satya164

This comment has been minimized.

Show comment
Hide comment
@satya164

satya164 Dec 19, 2015

Collaborator

@greghe Any updates on this?

Collaborator

satya164 commented Dec 19, 2015

@greghe Any updates on this?

@satya164

This comment has been minimized.

Show comment
Hide comment
@satya164

satya164 Dec 27, 2015

Collaborator

Closing in favour of #4975 which also add Android support.

Collaborator

satya164 commented Dec 27, 2015

Closing in favour of #4975 which also add Android support.

@satya164 satya164 closed this Dec 27, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment