Skip to content

Conversation

kevinstumpf
Copy link

image

image

@facebook-github-bot
Copy link
Contributor

By analyzing the blame information on this pull request, we identified @kmagiera, @janicduplessis and @dgladkov to be potential reviewers.

@facebook-github-bot
Copy link
Contributor

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla - and if you have received this in error or have any questions, please drop us a line at cla@fb.com. Thanks!

@kevinstumpf kevinstumpf changed the title Android support for a perspective transform Android support for perspective transform Feb 26, 2016
@facebook-github-bot
Copy link
Contributor

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

@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 Feb 26, 2016
@@ -149,6 +152,7 @@ public void setAccessibilityLiveRegion(T view, String liveRegion) {
}
}

private static final float MatrixPerspectiveToCameraDistanceMultiplier = 5;
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: This should probably be written LIKE_THE_OTHER_CONSTANTS and put at the top of the file.

@janicduplessis
Copy link
Contributor

This looks great, thanks for working on this!

cc @mkonicek

@@ -164,6 +168,16 @@ private static void setTransformMatrix(View view, ReadableMap matrix) {
(float) matrix.getDouble(PROP_DECOMPOSED_MATRIX_SCALE_X));
view.setScaleY(
(float) matrix.getDouble(PROP_DECOMPOSED_MATRIX_SCALE_Y));

if(matrix.hasKey(PROP_DECOMPOSED_MATRIX_PERSPECTIVE)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Put a space after if keywords.

@facebook-github-bot
Copy link
Contributor

@kevinstumpf updated the pull request.

@kevinstumpf
Copy link
Author

@janicduplessis -- thanks for the fast and thorough review. Addressed your comments and pushed an update. Thanks!

@nicklockwood
Copy link
Contributor

cc @dmmiller

@mkonicek
Copy link
Contributor

mkonicek commented Mar 4, 2016

Thanks for adding a great summary on the PR @kevinstumpf.

Does the perspective prop only allow to flip the view along the vertical axis (like in the flip-view example) or also the vertical one?

@facebook-github-bot shipit

@kevinstumpf
Copy link
Author

Does the perspective prop only allow to flip the view along the vertical axis (like in the flip-view example) or also the vertical one?

The perspective prop allows to flip the view along both the vertical and the horizontal axis. The react-native-flip-view module actually supports both (by specifying the "flipAxis" prop).

Thanks!

@chirag04
Copy link
Contributor

chirag04 commented Mar 4, 2016

@kevinstumpf This is awesome. One think i was really stuck with when building flip on android was backfaceVisibility. Wondering if you figured your way around it.

@kevinstumpf
Copy link
Author

@chirag04 I did figure a way around backfaceVisibility. I will submit a PR that adds android support over the next few days!

@kevinstumpf
Copy link
Author

@mkonicek I see that this PR is still labeled as "review-needed", despite your msg to the github bot. Is there anything left on my end to satisfy the bot? (I'm seeing that the branch is out-of-date with the base branch -- do I need to update it or will the bot take care of that automatically?)

Thanks!

@facebook-github-bot
Copy link
Contributor

@kevinstumpf updated the pull request.

@chirag04
Copy link
Contributor

@mkonicek seems like your import failed. Can you ship again if everything seems right.

@facebook-github-bot
Copy link
Contributor

@kevinstumpf updated the pull request.

1 similar comment
@facebook-github-bot
Copy link
Contributor

@kevinstumpf updated the pull request.

@mkonicek
Copy link
Contributor

Thanks for the ping @chirag04!

The bot will now add the Accepted label so we can find PRs with the label and they don't get forgotten about.

@mkonicek
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 3e760f4 Mar 15, 2016
@kevinstumpf
Copy link
Author

@chirag04 -- I just got around to submitting a PR that solves the backfaceVisibility issue on Android you had mentioned: #6695

ghost pushed a commit that referenced this pull request Mar 31, 2016
Summary:This reverts "Android support for perspective transform": 3e760f4, PR #6159, D3053532.

It broke negative scale transforms: #6622

Reviewed By: bestander

Differential Revision: D3120627

fb-gh-sync-id: 727528d39c049180fe4862d006f2089c997afd45
fbshipit-source-id: 727528d39c049180fe4862d006f2089c997afd45
@chirag04
Copy link
Contributor

@kevinstumpf This commit was reverted. can you take a look here: ae3dad8

@mkonicek
Copy link
Contributor

Thanks for commenting @chirag04!

@kevinstumpf Happy to merge a new PR with an updated test plan and ideally with a test.

@kevinstumpf
Copy link
Author

Thanks @chirag04 and @mkonicek. I addressed the issue with this PR: #6926

zebulgar pushed a commit to nightingale/react-native that referenced this pull request Jun 18, 2016
Summary:This reverts "Android support for perspective transform": 3e760f4, PR facebook#6159, D3053532.

It broke negative scale transforms: facebook#6622

Reviewed By: bestander

Differential Revision: D3120627

fb-gh-sync-id: 727528d39c049180fe4862d006f2089c997afd45
fbshipit-source-id: 727528d39c049180fe4862d006f2089c997afd45
This pull request was closed.
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.

6 participants