Skip to content
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

[Image] Added onLoadingStart/onLoadingFinish/onLoadingError props #604

Closed
wants to merge 6 commits into from

Conversation

jordanna
Copy link
Contributor

@jordanna jordanna commented Apr 2, 2015

Intended to provide loading states for network image sources. Updated the Image component example in the UIExplorer app to demonstrate how they can be used to fade in a network image (using the Animation API). Fixes #98.

…age component, which are intended to provide loading states for network image sources. Updated the Image component example in the UIExplorer app to demonstrate how they can be used to fade in a network image (using the Animation API). Fixes facebook#98.
@@ -88,6 +90,33 @@ var Image = React.createClass({
* testing scripts.
*/
testID: PropTypes.string,
/**
* onLoadingStart - Event callback function to invoke when image load begins.
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: can you remove onLoadingStart - from the beginning. We're displaying it twice on the docs and I sent an internal diff to remove the other ones from this file

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

@vjeux
Copy link
Contributor

vjeux commented Apr 2, 2015

This is awesome! Thanks for doing this! @a2, @nicklockwood can you review?

@brentvatne
Copy link
Collaborator

👍 this would be incredibly useful!

… the image loading callback functions (instead of merging). Updated the docs to avoid duplicating prop names.
@frantic
Copy link
Contributor

frantic commented Apr 2, 2015

👍 That's awesome!

@philikon
Copy link
Contributor

philikon commented Apr 2, 2015

Do we want to stay close to the web here and name them onLoad, onError, and, uh, I guess onLoading for when loading starts?

@@ -282,6 +298,9 @@ var styles = StyleSheet.create({
width: 38,
height: 38,
},
hidden: {
opacity: 0
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: missing a trailing comma

@vjeux
Copy link
Contributor

vjeux commented Apr 3, 2015

Sorry about the review delay, if you rename the event names i'll pull it in :)

@meetwudi
Copy link
Contributor

meetwudi commented Apr 3, 2015

This is awesome. One further suggestion, I think it would be better with documentation updated all at once :)

@jordanna
Copy link
Contributor Author

jordanna commented Apr 3, 2015

Sure not a problem, I can update docs/Image.md along with the other suggestions. Thanks!

jordanna added 2 commits April 3, 2015 14:10
…TML for load state notifications: onLoadStart, onLoad, onError. Added a section in the Image component docs about the feature including code snippets for a simple image fade-in and using a loading indicator as a placeholder while a large image downloads.
@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 Apr 7, 2015
@brentvatne
Copy link
Collaborator

Any update on this one? I'd like to use these features

cc @a2 @vjeux

@binlaniua
Copy link

👍👍 i need it

@binlaniua
Copy link

@jordanna maybe you miss RCT_EXPORT_MODULE() in the RCTNetworkImageViewManager.m

@brentvatne brentvatne changed the title Added onLoadingStart/onLoadingFinish/onLoadingError props to Image component [Image] Added onLoadingStart/onLoadingFinish/onLoadingError props Jun 1, 2015
@nicklockwood
Copy link
Contributor

This is now implemented.

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.

Image component onLoadingStart/onLoadingFinish/onLoadingError props
9 participants