[Image] Add onLoadX support on Android #3791

brentvatne · 2015-10-30T05:38:04Z

~~This is a WIP, just finished the first bit and wanted to get some feedback to see if this approach seems appropriate, as I haven't done a lot of Android development.~~

Looks ready for review now.

facebook-github-bot · 2015-10-30T05:38:22Z

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

mkonicek · 2015-10-30T08:49:44Z

Cool! I'll pass this to Tyrone who works on Fresco to take a look.

mkonicek · 2015-10-30T08:52:32Z

Should we make it consistent with iOS which has onLoadStart, onLoadEnd etc.?
https://facebook.github.io/react-native/docs/image.html

kmagiera · 2015-10-30T10:15:29Z

ReactAndroid/src/main/java/com/facebook/react/views/image/events/ImageLoadedEvent.java

do you plan to actually pass some data here? Otherwise it would be more efficient to send null instead of creating empty map

it looks like on iOS we just pass null, so unless someone can think of a good reason to do otherwise I'll change it to null here as well

facebook-github-bot · 2015-10-30T21:04:28Z

@brentvatne updated the pull request.

facebook-github-bot · 2015-10-30T22:53:18Z

@brentvatne updated the pull request.

facebook-github-bot · 2015-10-30T22:57:19Z

@brentvatne updated the pull request.

facebook-github-bot · 2015-10-30T23:23:17Z

@brentvatne updated the pull request.

facebook-github-bot · 2015-10-30T23:29:14Z

@brentvatne updated the pull request.

brentvatne · 2015-10-30T23:29:29Z

@mkonicek @tyronen @kmagiera - thanks for the great tips! I've factored them in and updated this PR. I also added an example to UIExplorer. I don't write a lot of Java so let me know if I did something weird and what you would suggest I do instead, please!

At the moment it supports: onLoadStart, onLoadEnd and onLoad. I added a comment about the absence of onError and onProgress being missing -- we can circle back and add those when support exists in fresco if that sounds good to you guys.

satya164 · 2015-11-02T22:02:23Z

+1

kmagiera · 2015-11-03T15:32:52Z

Libraries/Image/Image.ios.js

you mentioned onError isn't working on android, no?

correct -- this line is actually removing the platform annotation for onLoad

kmagiera · 2015-11-03T15:56:56Z

Getting there - few more comments

mkonicek · 2015-11-07T13:54:57Z

Thanks for working on this!

brentvatne · 2015-11-07T17:44:46Z

@kmagiera - just on vacation atm so not as much time but I will try to get to this over the next few days :) thanks for the review!

kmagiera · 2015-11-18T09:48:23Z

Looks great. Just a few more things outlined above. Thanks for your patience

facebook-github-bot · 2015-11-18T18:20:21Z

@brentvatne updated the pull request.

brentvatne · 2015-11-18T18:22:03Z

@kmagiera @mkonicek - feedback addressed, thanks guys!

facebook-github-bot · 2015-11-18T18:23:24Z

@brentvatne updated the pull request.

kmagiera · 2015-11-19T00:32:11Z

Great job, this PR deserves some cool gif

@facebook-github-bot shipit

facebook-github-bot · 2015-11-19T00:32:25Z

Thanks for importing. If you are an FB employee go to https://our.intern.facebook.com/intern/opensource/github/pull_request/550311681783433/int_phab to review.

mkonicek · 2015-11-19T19:50:55Z

Failed to land due to a Flow error. You can also see a repro for that on Travis. I asked the Flow guys about the correct way to fix this.

…ve prop

brentvatne · 2015-11-19T21:29:55Z

@mkonicek - fixed that and also noticed that f2545ba required me to add the shouldNotifyLoadEvents property to the new cfg at the bottom of Image.android.js

facebook-github-bot · 2015-11-19T21:30:25Z

@brentvatne updated the pull request.

mkonicek · 2015-11-20T13:23:48Z

@facebook-github-bot shipit

facebook-github-bot · 2015-11-20T13:24:29Z

Thanks for importing. If you are an FB employee go to https://our.intern.facebook.com/intern/opensource/github/pull_request/550311681783433/int_phab to review.

mkonicek · 2015-11-26T17:35:47Z

Finally landed 🎉

The Android PRs often require changes to Buck files in order for them to build internally. This makes merging them quite a bit of work :( Some ideas how to fix this:

Have one Buck file for all of OSS RN instead of many granular Buck files
Open source all Buck files and report failures back to github (PITA)

Summary: ~~This is a WIP, just finished the first bit and wanted to get some feedback to see if this approach seems appropriate, as I haven't done a lot of Android development.~~ Looks ready for review now. Closes facebook#3791 Reviewed By: svcscm Differential Revision: D2672262 Pulled By: mkonicek fb-gh-sync-id: 1e8f1cc6658fb719a68f7da455f30a7c9b1db730

vjeux · 2016-05-10T19:37:05Z

ReactAndroid/src/main/java/com/facebook/react/views/image/ReactImageView.java

+        }
+
+        @Override
+        public void onFailure(String id, Throwable throwable) {


You are saying that fresco doesn't support ON_ERROR but you have onFailure here. Is the comment not correct? Any reason why you don't throw ON_ERROR here?

facebook-github-bot added GH Review: review-needed CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. labels Oct 30, 2015

kmagiera reviewed Oct 30, 2015
View reviewed changes

kmagiera reviewed Nov 3, 2015
View reviewed changes

kmagiera added GH Review: needs-revision and removed GH Review: review-needed labels Nov 3, 2015

kmagiera added GH Review: needs-revision and removed GH Review: review-needed labels Nov 18, 2015

brentvatne added 4 commits November 18, 2015 10:19

[Image] Add onLoad support on Android

c6eb3d4

[Image] Add comment about casting mEventType

472aa7d

[Image] Add space between (short) and mEventType

0a229ed

Incorporate feedback from Martin and Krzysztof

61257e6

facebook-github-bot added GH Review: review-needed and removed GH Review: needs-revision labels Nov 18, 2015

Remove unnecessary line

a0553f2

Fix flow error in ImageExample and add shouldNotifyLoadEvents to nati…

1c014a2

…ve prop

brentvatne closed this in ae09a10 Nov 26, 2015

vjeux reviewed May 10, 2016
View reviewed changes

brentvatne mentioned this pull request Aug 15, 2016

[Android] Add missing onError event to Image #9416

Closed

[Image] Add onLoadX support on Android #3791

[Image] Add onLoadX support on Android #3791

Uh oh!

Conversation

brentvatne commented Oct 30, 2015

Uh oh!

facebook-github-bot commented Oct 30, 2015

Uh oh!

mkonicek commented Oct 30, 2015

Uh oh!

mkonicek commented Oct 30, 2015

Uh oh!

kmagiera Oct 30, 2015

Choose a reason for hiding this comment

Uh oh!

brentvatne Oct 30, 2015

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot commented Oct 30, 2015

Uh oh!

facebook-github-bot commented Oct 30, 2015

Uh oh!

facebook-github-bot commented Oct 30, 2015

Uh oh!

facebook-github-bot commented Oct 30, 2015

Uh oh!

facebook-github-bot commented Oct 30, 2015

Uh oh!

brentvatne commented Oct 30, 2015

Uh oh!

satya164 commented Nov 2, 2015

Uh oh!

kmagiera Nov 3, 2015

Choose a reason for hiding this comment

Uh oh!

brentvatne Nov 17, 2015

Choose a reason for hiding this comment

Uh oh!

kmagiera commented Nov 3, 2015

Uh oh!

mkonicek commented Nov 7, 2015

Uh oh!

brentvatne commented Nov 7, 2015

Uh oh!

kmagiera commented Nov 18, 2015

Uh oh!

facebook-github-bot commented Nov 18, 2015

Uh oh!

brentvatne commented Nov 18, 2015

Uh oh!

facebook-github-bot commented Nov 18, 2015

Uh oh!

kmagiera commented Nov 19, 2015

Uh oh!

facebook-github-bot commented Nov 19, 2015

Uh oh!

mkonicek commented Nov 19, 2015

Uh oh!

brentvatne commented Nov 19, 2015

Uh oh!

facebook-github-bot commented Nov 19, 2015

Uh oh!

mkonicek commented Nov 20, 2015

Uh oh!

facebook-github-bot commented Nov 20, 2015

Uh oh!

mkonicek commented Nov 26, 2015

Uh oh!

vjeux May 10, 2016

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants