Use elevation to implement shadows on Android #4180

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
8 participants
@corbt
Contributor

corbt commented Nov 17, 2015

This PR includes a working interface to Android's elevation property implemented as an RN style. Elevation is the only (easy) platform-supported way to create shadows. For it to work note that you must be running on Android 5.0+, and add elevation to a view with a backgroundColor set. These are platform limitations.

This PR is not intended to be merged in its current state, but rather to inform the discussion from #2768. At a minimum, before merging we would need to add the elevation style to the docs and rebase this to master (EDIT I have now rebased on master because from v0.14.2 too many commits were being pulled in -- haven't tested it since the rebase though). Additionally, it might be good to add tests, although I couldn't find any for the Android code. I'm happy to get that done if this feature gets signed off by the React Native team.

Finally, as I argued in #2768 I think that elevation is a useful abstraction over manual shadow management, and I would like to see it ported over to the iOS side as well. This would allow cross-platform developers to create shadows with a single elevation style. The alternative would be to just keep the shadow* APIs on iOS and elevation for Android, which to me feels less clean. However, this could be merged before that work is done, and the cross-platform implementation can come later.

Example usage:

var RNelevation = React.createClass({
  render: function() {
    return (
      <View style={styles.container}>
        <Text style={styles.title}>
          React Native Elevation Test
        </Text>
        <View style={styles.bigBox}>
          <View style={styles.smallBox} />
        </View>
      </View>
    );
  }
});

var styles = StyleSheet.create({
  container: {
    flex: 1,
    justifyContent: 'center',
    alignItems: 'center',
    backgroundColor: '#F5FCFF',
  },
  title: {
    fontSize: 20,
    textAlign: 'center',
    margin: 10,
  },
  bigBox: {
    width: 200,
    height: 200,
    elevation: 20,
    backgroundColor: 'green'
  },
  smallBox: {
    backgroundColor: 'red',
    width: 100,
    height: 100,
    elevation: 10
  }
});

Result:
rn elevation

@facebook-github-bot

This comment has been minimized.

Show comment
Hide comment
@facebook-github-bot

facebook-github-bot Nov 17, 2015

By analyzing the blame information on this pull request, we identified @martinbigio, @tadeuzagallo and @ide to be potential reviewers.

By analyzing the blame information on this pull request, we identified @martinbigio, @tadeuzagallo and @ide to be potential reviewers.

@facebook-github-bot

This comment has been minimized.

Show comment
Hide comment
@facebook-github-bot

facebook-github-bot Nov 17, 2015

@corbt updated the pull request.

@corbt updated the pull request.

@facebook-github-bot

This comment has been minimized.

Show comment
Hide comment
@facebook-github-bot

facebook-github-bot Nov 17, 2015

@corbt updated the pull request.

@corbt updated the pull request.

+ : 0;
+
+ outline.setRoundRect(getBounds(), borderRadius+extraRadiusFromBorderWidth);
+ }

This comment has been minimized.

@mkonicek

mkonicek Nov 17, 2015

Contributor

Can you please add a comment explaining why this is needed? Why is it needed? Isn't setting elevation enough?

@mkonicek

mkonicek Nov 17, 2015

Contributor

Can you please add a comment explaining why this is needed? Why is it needed? Isn't setting elevation enough?

+ ? mBorderWidth.get(Spacing.ALL)/2
+ : 0;
+
+ outline.setRoundRect(getBounds(), borderRadius+extraRadiusFromBorderWidth);

This comment has been minimized.

@mkonicek

mkonicek Nov 17, 2015

Contributor

nit: spaces around +

@mkonicek

mkonicek Nov 17, 2015

Contributor

nit: spaces around +

This comment has been minimized.

@corbt

corbt Nov 17, 2015

Contributor

sorry, not sure what this comment means?

@corbt

corbt Nov 17, 2015

Contributor

sorry, not sure what this comment means?

This comment has been minimized.

@corbt

corbt Nov 17, 2015

Contributor

ok, figured it out.

@corbt

corbt Nov 17, 2015

Contributor

ok, figured it out.

@@ -64,6 +65,11 @@ public void setBorderStyle(ReactViewGroup view, @Nullable String borderStyle) {
view.setBorderStyle(borderStyle);
}
+ @ReactProp(name = "elevation")
+ public void setElevation(ReactViewGroup view, float elevation) {
+ ViewCompat.setElevation(view, PixelUtil.toPixelFromDIP(elevation));

This comment has been minimized.

@mkonicek

mkonicek Nov 17, 2015

Contributor

Why not check API level and call View.setElevation? http://developer.android.com/reference/android/view/View.html#setElevation(float)

This comment has been minimized.

@corbt

corbt Nov 17, 2015

Contributor

I suspect that's essentially what ViewCompat.setElevation does, with the addition that if Android ever does backport elevation to earlier APIs (admittedly unlikely) it'll start working there as well. But yeah, doing it the other way would work fine too. Is there a reason to prefer one style over the other?

@corbt

corbt Nov 17, 2015

Contributor

I suspect that's essentially what ViewCompat.setElevation does, with the addition that if Android ever does backport elevation to earlier APIs (admittedly unlikely) it'll start working there as well. But yeah, doing it the other way would work fine too. Is there a reason to prefer one style over the other?

@mkonicek

This comment has been minimized.

Show comment
Hide comment
@mkonicek

mkonicek Nov 17, 2015

Contributor

Additionally, it might be good to add tests, although I couldn't find any for the Android code.

Absolutely, we plan to open source the Android integration tests.

Contributor

mkonicek commented Nov 17, 2015

Additionally, it might be good to add tests, although I couldn't find any for the Android code.

Absolutely, we plan to open source the Android integration tests.

@mkonicek

This comment has been minimized.

Show comment
Hide comment
@mkonicek

mkonicek Nov 17, 2015

Contributor

Totally agree we can add elevation now and decide on a cross-platform API for shadows later. One thing that's clear is that we shouldn't be trying to infer the elevation from the shadow properties. Therefore starting by adding elevation is a good way to go.

Contributor

mkonicek commented Nov 17, 2015

Totally agree we can add elevation now and decide on a cross-platform API for shadows later. One thing that's clear is that we shouldn't be trying to infer the elevation from the shadow properties. Therefore starting by adding elevation is a good way to go.

@mkonicek

This comment has been minimized.

Show comment
Hide comment
@mkonicek

mkonicek Nov 17, 2015

Contributor

Thanks for much for working on this! 👍

I've read briefly through #2768 but the discussion is quite long, I think it'll be helpful to discuss directly here now we know adding an elevation prop is the way to go :)

Contributor

mkonicek commented Nov 17, 2015

Thanks for much for working on this! 👍

I've read briefly through #2768 but the discussion is quite long, I think it'll be helpful to discuss directly here now we know adding an elevation prop is the way to go :)

@facebook-github-bot

This comment has been minimized.

Show comment
Hide comment
@facebook-github-bot

facebook-github-bot Nov 17, 2015

@corbt updated the pull request.

@corbt updated the pull request.

@corbt

This comment has been minimized.

Show comment
Hide comment
@corbt

corbt Nov 17, 2015

Contributor

Ok, I've pushed some changes related to @mkonicek's review. As far as why getOutline needs to be implemented: unfortunately I haven't been able to find any documentation that explains the requirement (or even the setElevation source code that might point to where the outline is used). But intuitively, it makes sense that to cast a shadow around an object you'd need to know where its bounds are. And practically speaking, it's there because when I get rid of it the shadow stops working. :) Anyway, I added a brief comment mentioning its relationship to elevation.

Contributor

corbt commented Nov 17, 2015

Ok, I've pushed some changes related to @mkonicek's review. As far as why getOutline needs to be implemented: unfortunately I haven't been able to find any documentation that explains the requirement (or even the setElevation source code that might point to where the outline is used). But intuitively, it makes sense that to cast a shadow around an object you'd need to know where its bounds are. And practically speaking, it's there because when I get rid of it the shadow stops working. :) Anyway, I added a brief comment mentioning its relationship to elevation.

@corbt

This comment has been minimized.

Show comment
Hide comment
@corbt

corbt Nov 17, 2015

Contributor

I should mention that this PR only adds elevation to the View object, nothing else. Would it make sense to add explicit support for it to other UI components, or just require people to wrap anything they want shadows on inside a View? How does it work right now on iOS for shadows?

EDIT: it appears that the border* styles on Android are only supported on Views and ImageViews. Am I interpreting the code right? If so, it may make sense to add elevation support to just those two classes; borders and shadows are similar-ish stylistic concerns.

Contributor

corbt commented Nov 17, 2015

I should mention that this PR only adds elevation to the View object, nothing else. Would it make sense to add explicit support for it to other UI components, or just require people to wrap anything they want shadows on inside a View? How does it work right now on iOS for shadows?

EDIT: it appears that the border* styles on Android are only supported on Views and ImageViews. Am I interpreting the code right? If so, it may make sense to add elevation support to just those two classes; borders and shadows are similar-ish stylistic concerns.

@mkonicek

This comment has been minimized.

Show comment
Hide comment
@mkonicek

mkonicek Nov 17, 2015

Contributor

Thanks for adding the comment to getOutline! Just to be safe, have you tested setting elevation on API < 21? It should have no effect and not crash :)

it appears that the border* styles on Android are only supported on Views and ImageViews.

Yes this is correct. Yes, adding elevation to those two (and even better starting just with View in this PR) sounds good.

Contributor

mkonicek commented Nov 17, 2015

Thanks for adding the comment to getOutline! Just to be safe, have you tested setting elevation on API < 21? It should have no effect and not crash :)

it appears that the border* styles on Android are only supported on Views and ImageViews.

Yes this is correct. Yes, adding elevation to those two (and even better starting just with View in this PR) sounds good.

+ float borderRadius = (!CSSConstants.isUndefined(mBorderRadius) && mBorderRadius > 0)
+ ? mBorderRadius
+ : 0;
+ float extraRadiusFromBorderWidth = (mBorderWidth != null)

This comment has been minimized.

@mkonicek

mkonicek Nov 17, 2015

Contributor

Is this needed? How does the shadow look without it? Maybe we just need higher elevation?

@mkonicek

mkonicek Nov 17, 2015

Contributor

Is this needed? How does the shadow look without it? Maybe we just need higher elevation?

+ ? mBorderRadius
+ : 0;
+ float extraRadiusFromBorderWidth = (mBorderWidth != null)
+ ? mBorderWidth.get(Spacing.ALL)/2

This comment has been minimized.

@mkonicek

mkonicek Nov 17, 2015

Contributor

What if there's only the bottom border for example?

@mkonicek

mkonicek Nov 17, 2015

Contributor

What if there's only the bottom border for example?

@mkonicek

This comment has been minimized.

Show comment
Hide comment
@mkonicek

mkonicek Nov 17, 2015

Contributor

Can you please provide some screenshots for views with thick borders / round corners / thick border only on one side etc.?

Contributor

mkonicek commented Nov 17, 2015

Can you please provide some screenshots for views with thick borders / round corners / thick border only on one side etc.?

@brentvatne

This comment has been minimized.

Show comment
Hide comment
@brentvatne

brentvatne Nov 17, 2015

Collaborator

This is fantastic! Looking forward to getting it in, thanks @corbt and @mkonicek! 😄 🚀

Collaborator

brentvatne commented Nov 17, 2015

This is fantastic! Looking forward to getting it in, thanks @corbt and @mkonicek! 😄 🚀

@corbt

This comment has been minimized.

Show comment
Hide comment
@corbt

corbt Nov 18, 2015

Contributor

@mkonicek ok I'm getting distracted by some real-world work 😉; I'll get those screenshots for you but probably on Thursday.

Contributor

corbt commented Nov 18, 2015

@mkonicek ok I'm getting distracted by some real-world work 😉; I'll get those screenshots for you but probably on Thursday.

@mkonicek

This comment has been minimized.

Show comment
Hide comment
@mkonicek

mkonicek Nov 18, 2015

Contributor

Haha no problem @corbt! I was curious why extraRadiusFromBorderWidth was needed and how that works when borders have uneven widths on each side.

Contributor

mkonicek commented Nov 18, 2015

Haha no problem @corbt! I was curious why extraRadiusFromBorderWidth was needed and how that works when borders have uneven widths on each side.

@corbt

This comment has been minimized.

Show comment
Hide comment
@corbt

corbt Nov 19, 2015

Contributor

Ok, find some screen shots of different border/radius combinations below.

I've also verified that nothing happens on API < 21 (checked in an emulator on API 16), and updated the PR to fix a bug where there is a large border and no border radius.

Note that there's no special handling for borderBottomWidth and friends because it appears that RN ignores them when a borderRadius is applied, so we should ignore them here as well.

device-2015-11-19-160801

Code for the above:

class Example extends Component {
  render() {
    return <View style={styles.exampleContainer}>
      <View>
        <Text style={styles.subtitle}>{JSON.stringify(this.props.style, null, 2).slice(1, -1)}</Text>
      </View>
      <View style={[styles.example, this.props.style]} />
    </View>;
  }
}
        // <View style={{flexDirection: 'row'}}>

var RNelevation = React.createClass({
  render: function() {
    return (
      <View style={styles.container}>
        <Example style={{borderBottomWidth: 30}} />
        <Example style={{borderRadius: 45}} />
        <Example style={{borderRadius: 30, borderWidth: 30}} />
        <Example style={{borderWidth: 5, borderBottomWidth: 30}} />
        <Example style={{borderBottomWidth: 30, borderRadius: 30}} />
        <Example style={{borderWidth: 30, borderRadius: 1}} />
        <Example style={{borderWidth: 30}} />
      </View>
    );
  }
});

var styles = StyleSheet.create({
  container: {
    flex: 1,
    justifyContent: 'flex-start',
    alignItems: 'center',
    flexDirection: 'row',
    flexWrap: 'wrap',
  },
  exampleContainer: {
    alignItems: 'center',
    width: 200,
    margin: 20,
  },
  example: {
    width: 100,
    height: 100,
    backgroundColor: 'red',
    elevation: 10,
    margin: 50,
    marginTop: 5,
    borderColor: 'green'
  },
  subtitle: {
    textAlign: 'center',
  }
});
Contributor

corbt commented Nov 19, 2015

Ok, find some screen shots of different border/radius combinations below.

I've also verified that nothing happens on API < 21 (checked in an emulator on API 16), and updated the PR to fix a bug where there is a large border and no border radius.

Note that there's no special handling for borderBottomWidth and friends because it appears that RN ignores them when a borderRadius is applied, so we should ignore them here as well.

device-2015-11-19-160801

Code for the above:

class Example extends Component {
  render() {
    return <View style={styles.exampleContainer}>
      <View>
        <Text style={styles.subtitle}>{JSON.stringify(this.props.style, null, 2).slice(1, -1)}</Text>
      </View>
      <View style={[styles.example, this.props.style]} />
    </View>;
  }
}
        // <View style={{flexDirection: 'row'}}>

var RNelevation = React.createClass({
  render: function() {
    return (
      <View style={styles.container}>
        <Example style={{borderBottomWidth: 30}} />
        <Example style={{borderRadius: 45}} />
        <Example style={{borderRadius: 30, borderWidth: 30}} />
        <Example style={{borderWidth: 5, borderBottomWidth: 30}} />
        <Example style={{borderBottomWidth: 30, borderRadius: 30}} />
        <Example style={{borderWidth: 30, borderRadius: 1}} />
        <Example style={{borderWidth: 30}} />
      </View>
    );
  }
});

var styles = StyleSheet.create({
  container: {
    flex: 1,
    justifyContent: 'flex-start',
    alignItems: 'center',
    flexDirection: 'row',
    flexWrap: 'wrap',
  },
  exampleContainer: {
    alignItems: 'center',
    width: 200,
    margin: 20,
  },
  example: {
    width: 100,
    height: 100,
    backgroundColor: 'red',
    elevation: 10,
    margin: 50,
    marginTop: 5,
    borderColor: 'green'
  },
  subtitle: {
    textAlign: 'center',
  }
});
@facebook-github-bot

This comment has been minimized.

Show comment
Hide comment
@facebook-github-bot

facebook-github-bot Nov 19, 2015

@corbt updated the pull request.

@corbt updated the pull request.

@mkonicek

This comment has been minimized.

Show comment
Hide comment
@mkonicek

mkonicek Nov 22, 2015

Contributor

Thanks for the explanation and the screenshots! We should add this an an example to the UI Explorer.

@facebook-github-bot shipit

Contributor

mkonicek commented Nov 22, 2015

Thanks for the explanation and the screenshots! We should add this an an example to the UI Explorer.

@facebook-github-bot shipit

@facebook-github-bot

This comment has been minimized.

Show comment
Hide comment
@satya164

This comment has been minimized.

Show comment
Hide comment
@satya164

satya164 Nov 22, 2015

Collaborator

Another property of elevation seems to be that it changes the stacking order. An element with elevation value 6 will be always above an element with elevation value 4.

That's one caveat if we want to have the same thing on iOS, as RN doesn't support changing the stacking order.

Collaborator

satya164 commented Nov 22, 2015

Another property of elevation seems to be that it changes the stacking order. An element with elevation value 6 will be always above an element with elevation value 4.

That's one caveat if we want to have the same thing on iOS, as RN doesn't support changing the stacking order.

@ide

This comment has been minimized.

Show comment
Hide comment
@ide

ide Nov 22, 2015

Collaborator

We should mention @satya164's point in a docblock for this property.

Collaborator

ide commented Nov 22, 2015

We should mention @satya164's point in a docblock for this property.

@corbt

This comment has been minimized.

Show comment
Hide comment
@corbt

corbt Nov 23, 2015

Contributor

I'm happy to add documentation, including @satya164's important observation that elevation affects stacking order. In fact I was planning on adding documentation to this PR before it was merged, as soon as there was a consensus that this was a good idea. :) Not sure about the mechanics of @facebook-github-bot -- now that this has been "shipped", should I still add that to this PR or open a new one?

Contributor

corbt commented Nov 23, 2015

I'm happy to add documentation, including @satya164's important observation that elevation affects stacking order. In fact I was planning on adding documentation to this PR before it was merged, as soon as there was a consensus that this was a good idea. :) Not sure about the mechanics of @facebook-github-bot -- now that this has been "shipped", should I still add that to this PR or open a new one?

+ outline.setRoundRect(getBounds(), mBorderRadius + extraRadiusFromBorderWidth);
+ }
+ else {
+ outline.setRect(getBounds());
@mkonicek

This comment has been minimized.

Show comment
Hide comment
@mkonicek

mkonicek Nov 23, 2015

Contributor

It hasn't landed yet due to Buck not seeing @NonNull. We use Buck to build the code internally. I could add that to the Buck file but suggest we just remove the annotation and import here. It makes the dependencies simpler and we don't use that annotation (even though here you're overriding the method so you technically should but well..). We just use javax.annotation.Nullable - we assume everything else is non-null by default.

Once it lands this PR would be automatically closed. You can still edit this PR and I'll comment 'shipit' again.

For details on how the bot works you can check slides 45+ in this presentation: https://speakerdeck.com/mkonicek/under-the-hood-of-react-native

Adding docs about the z-order is a great idea!

Contributor

mkonicek commented Nov 23, 2015

It hasn't landed yet due to Buck not seeing @NonNull. We use Buck to build the code internally. I could add that to the Buck file but suggest we just remove the annotation and import here. It makes the dependencies simpler and we don't use that annotation (even though here you're overriding the method so you technically should but well..). We just use javax.annotation.Nullable - we assume everything else is non-null by default.

Once it lands this PR would be automatically closed. You can still edit this PR and I'll comment 'shipit' again.

For details on how the bot works you can check slides 45+ in this presentation: https://speakerdeck.com/mkonicek/under-the-hood-of-react-native

Adding docs about the z-order is a great idea!

@facebook-github-bot

This comment has been minimized.

Show comment
Hide comment
@facebook-github-bot

facebook-github-bot Nov 23, 2015

@corbt updated the pull request.

@corbt updated the pull request.

@corbt

This comment has been minimized.

Show comment
Hide comment
@corbt

corbt Nov 23, 2015

Contributor

@mkonicek ok, removed the @NotNull, added the relevant call to super, and added some documentation. Unfortunately it looks like there isn't any way to expose docblocks for styles currently (which I guess makes sense for styles pulled directly from CSS, but would be nice for less obvious ones like this), but I tweaked "Known Issues" to describe this solution, and added a docblock to the styles page anyway in case rendering code for those is ever added to doc generation.

Contributor

corbt commented Nov 23, 2015

@mkonicek ok, removed the @NotNull, added the relevant call to super, and added some documentation. Unfortunately it looks like there isn't any way to expose docblocks for styles currently (which I guess makes sense for styles pulled directly from CSS, but would be nice for less obvious ones like this), but I tweaked "Known Issues" to describe this solution, and added a docblock to the styles page anyway in case rendering code for those is ever added to doc generation.

@mkonicek

This comment has been minimized.

Show comment
Hide comment
@mkonicek

mkonicek Nov 24, 2015

Contributor

Cool, nice work!

@facebook-github-bot shipit

Contributor

mkonicek commented Nov 24, 2015

Cool, nice work!

@facebook-github-bot shipit

@facebook-github-bot

This comment has been minimized.

Show comment
Hide comment
@mkonicek

This comment has been minimized.

Show comment
Hide comment
@mkonicek

mkonicek Nov 24, 2015

Contributor

Landing this now. Due to a limitation of how the code import script works we'll have to commit the changes to the docs separately once this lands (docs are only stored on github, not in the fb codebase, we should improve the script). I'm happy to commit the changes to the docs for you. Thanks for the patience! :)

Contributor

mkonicek commented Nov 24, 2015

Landing this now. Due to a limitation of how the code import script works we'll have to commit the changes to the docs separately once this lands (docs are only stored on github, not in the fb codebase, we should improve the script). I'm happy to commit the changes to the docs for you. Thanks for the patience! :)

@satya164

This comment has been minimized.

Show comment
Hide comment
@satya164

satya164 Nov 25, 2015

Collaborator

@mkonicek Will it go in 0.16?

Collaborator

satya164 commented Nov 25, 2015

@mkonicek Will it go in 0.16?

@ghost ghost closed this in b65f1f2 Nov 26, 2015

@mkonicek

This comment has been minimized.

Show comment
Hide comment
@mkonicek

mkonicek Nov 26, 2015

Contributor

Let's cherry-pick it.

Contributor

mkonicek commented Nov 26, 2015

Let's cherry-pick it.

@mkonicek

This comment has been minimized.

Show comment
Hide comment
@mkonicek

mkonicek Nov 26, 2015

Contributor

@corbt This is finally in 🎉 I've made a few small tweaks while landing it.

Contributor

mkonicek commented Nov 26, 2015

@corbt This is finally in 🎉 I've made a few small tweaks while landing it.

@mkonicek

This comment has been minimized.

Show comment
Hide comment
@mkonicek

mkonicek Nov 26, 2015

Contributor

Updated Known Issues. Don't think no cross-platform shadows are necessarily a limitation. Shadows work differently on each platform and I believe it's okay to use the platform-specific props: 0f89696

People might actually want shadows to be defined and behave in a way native to the platform. Let's ship this and get feedback.

Contributor

mkonicek commented Nov 26, 2015

Updated Known Issues. Don't think no cross-platform shadows are necessarily a limitation. Shadows work differently on each platform and I believe it's okay to use the platform-specific props: 0f89696

People might actually want shadows to be defined and behave in a way native to the platform. Let's ship this and get feedback.

@mkonicek

This comment has been minimized.

Show comment
Hide comment
@mkonicek

mkonicek Nov 26, 2015

Contributor

Thanks so much for working on this @corbt!

Contributor

mkonicek commented Nov 26, 2015

Thanks so much for working on this @corbt!

@corbt

This comment has been minimized.

Show comment
Hide comment
@corbt

corbt Nov 27, 2015

Contributor

Sweet, glad this is in! 👍

Contributor

corbt commented Nov 27, 2015

Sweet, glad this is in! 👍

@jcsmesquita

This comment has been minimized.

Show comment
Hide comment
@jcsmesquita

jcsmesquita Nov 27, 2015

Is it possible to use this now? Which version do I need to install? Great work btw 🚀

Is it possible to use this now? Which version do I need to install? Great work btw 🚀

@corbt

This comment has been minimized.

Show comment
Hide comment
@corbt

corbt Nov 27, 2015

Contributor

@jcsmesquita currently it looks like it's only in master, although @mkonicek said they might cherry-pick it into 0.16. So it'll either be in the 0.16 release or 0.17. But if you want to use it right now you'll need to clone master.

Contributor

corbt commented Nov 27, 2015

@jcsmesquita currently it looks like it's only in master, although @mkonicek said they might cherry-pick it into 0.16. So it'll either be in the 0.16 release or 0.17. But if you want to use it right now you'll need to clone master.

@jcsmesquita

This comment has been minimized.

Show comment
Hide comment
@jcsmesquita

jcsmesquita Nov 27, 2015

@corbt Got it :) thanks for the explanation!

@corbt Got it :) thanks for the explanation!

satya164 added a commit to callstack/react-native that referenced this pull request Dec 1, 2015

Use elevation to implement shadows on Android
Summary: This PR includes a working interface to Android's `elevation` property implemented as an RN style. Elevation is the only (easy) [platform-supported way to create shadows](http://developer.android.com/training/material/shadows-clipping.html). For it to work note that you must be running on Android 5.0+, and add `elevation` to a view with a `backgroundColor` set. These are platform limitations.

This PR is not intended to be merged in its current state, but rather to inform the discussion from #2768. At a minimum, before merging we would need to add the elevation style to the docs and rebase this to master (**EDIT** I have now rebased on master because from v0.14.2 too many commits were being pulled in -- haven't tested it since the rebase though). Additionally, it might be good to add tests, although I couldn't find any for the Android code. I'm happy to get that done if this feature gets signed off by the React Native team.

Finally, as I argued in #2768 I think that `elevation` is a useful abstraction ov
Closes facebook#4180

Reviewed By: svcscm

Differential Revision: D2684746

Pulled By: mkonicek

fb-gh-sync-id: 825f3ccd20c4b0eea9d11b5f0e3a6b018b7e4378

sunnylqm added a commit to sunnylqm/react-native that referenced this pull request Dec 2, 2015

Use elevation to implement shadows on Android
Summary: This PR includes a working interface to Android's `elevation` property implemented as an RN style. Elevation is the only (easy) [platform-supported way to create shadows](http://developer.android.com/training/material/shadows-clipping.html). For it to work note that you must be running on Android 5.0+, and add `elevation` to a view with a `backgroundColor` set. These are platform limitations.

This PR is not intended to be merged in its current state, but rather to inform the discussion from #2768. At a minimum, before merging we would need to add the elevation style to the docs and rebase this to master (**EDIT** I have now rebased on master because from v0.14.2 too many commits were being pulled in -- haven't tested it since the rebase though). Additionally, it might be good to add tests, although I couldn't find any for the Android code. I'm happy to get that done if this feature gets signed off by the React Native team.

Finally, as I argued in #2768 I think that `elevation` is a useful abstraction ov
Closes facebook#4180

Reviewed By: svcscm

Differential Revision: D2684746

Pulled By: mkonicek

fb-gh-sync-id: 825f3ccd20c4b0eea9d11b5f0e3a6b018b7e4378

satya164 added a commit to callstack/react-native that referenced this pull request Dec 4, 2015

Use elevation to implement shadows on Android
Summary: This PR includes a working interface to Android's `elevation` property implemented as an RN style. Elevation is the only (easy) [platform-supported way to create shadows](http://developer.android.com/training/material/shadows-clipping.html). For it to work note that you must be running on Android 5.0+, and add `elevation` to a view with a `backgroundColor` set. These are platform limitations.

This PR is not intended to be merged in its current state, but rather to inform the discussion from #2768. At a minimum, before merging we would need to add the elevation style to the docs and rebase this to master (**EDIT** I have now rebased on master because from v0.14.2 too many commits were being pulled in -- haven't tested it since the rebase though). Additionally, it might be good to add tests, although I couldn't find any for the Android code. I'm happy to get that done if this feature gets signed off by the React Native team.

Finally, as I argued in #2768 I think that `elevation` is a useful abstraction ov
Closes facebook#4180

Reviewed By: svcscm

Differential Revision: D2684746

Pulled By: mkonicek

fb-gh-sync-id: 825f3ccd20c4b0eea9d11b5f0e3a6b018b7e4378

mkonicek added a commit that referenced this pull request Dec 4, 2015

Use elevation to implement shadows on Android
Summary: This PR includes a working interface to Android's `elevation` property implemented as an RN style. Elevation is the only (easy) [platform-supported way to create shadows](http://developer.android.com/training/material/shadows-clipping.html). For it to work note that you must be running on Android 5.0+, and add `elevation` to a view with a `backgroundColor` set. These are platform limitations.

This PR is not intended to be merged in its current state, but rather to inform the discussion from #2768. At a minimum, before merging we would need to add the elevation style to the docs and rebase this to master (**EDIT** I have now rebased on master because from v0.14.2 too many commits were being pulled in -- haven't tested it since the rebase though). Additionally, it might be good to add tests, although I couldn't find any for the Android code. I'm happy to get that done if this feature gets signed off by the React Native team.

Finally, as I argued in #2768 I think that `elevation` is a useful abstraction ov
Closes #4180

Reviewed By: svcscm

Differential Revision: D2684746

Pulled By: mkonicek

fb-gh-sync-id: 825f3ccd20c4b0eea9d11b5f0e3a6b018b7e4378

ghost pushed a commit that referenced this pull request Dec 9, 2015

Fix bug in Android elevation implementation
Summary:
If border radius is not set or is zero, then elevation will not
work properly. This bug seems to have been introduced when the
style in facebook/react-native#4180 was modified slightly to
produce commit b65f1f2.
Closes #4555

Reviewed By: svcscm

Differential Revision: D2741203

Pulled By: mkonicek

fb-gh-sync-id: f4ee9ccdfc64374d58824a6e988409ac2b7532a4

Crash-- added a commit to Crash--/react-native that referenced this pull request Dec 24, 2015

Use elevation to implement shadows on Android
Summary: This PR includes a working interface to Android's `elevation` property implemented as an RN style. Elevation is the only (easy) [platform-supported way to create shadows](http://developer.android.com/training/material/shadows-clipping.html). For it to work note that you must be running on Android 5.0+, and add `elevation` to a view with a `backgroundColor` set. These are platform limitations.

This PR is not intended to be merged in its current state, but rather to inform the discussion from #2768. At a minimum, before merging we would need to add the elevation style to the docs and rebase this to master (**EDIT** I have now rebased on master because from v0.14.2 too many commits were being pulled in -- haven't tested it since the rebase though). Additionally, it might be good to add tests, although I couldn't find any for the Android code. I'm happy to get that done if this feature gets signed off by the React Native team.

Finally, as I argued in #2768 I think that `elevation` is a useful abstraction ov
Closes facebook#4180

Reviewed By: svcscm

Differential Revision: D2684746

Pulled By: mkonicek

fb-gh-sync-id: 825f3ccd20c4b0eea9d11b5f0e3a6b018b7e4378

@corbt corbt deleted the corbt:view_elevation branch Feb 3, 2016

@Kishanjvaghela

This comment has been minimized.

Show comment
Hide comment
@Kishanjvaghela

Kishanjvaghela Apr 28, 2017

I have implemented CardView for react-native with elevation, that support android(All version) and iOS. Let me know is it help you or not. https://github.com/Kishanjvaghela/react-native-cardview

I have implemented CardView for react-native with elevation, that support android(All version) and iOS. Let me know is it help you or not. https://github.com/Kishanjvaghela/react-native-cardview

This issue was closed.

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