Skip to content

Conversation

oliviabanis
Copy link

Simple documentation errata.

@facebook-github-bot
Copy link
Contributor

By analyzing the blame information on this pull request, we identified @davidaurelio and @JoelMarcey 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!

@ghost
Copy link

ghost commented Sep 25, 2016

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

@ghost ghost 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 Sep 25, 2016
@@ -34,15 +34,15 @@ And `button.js` code contains
<Image source={require('./img/check.png')} />
```

Packager will bundle and serve the image corresponding to device's screen density, e.g. on iPhone 5s `check@2x.png` will be used, on Nexus 5 – `check@3x.png`. If there is no image matching the screen density, the closest best option will be selected.
Packager will bundle and serve the image corresponding to the device's screen density, e.g. on iPhone 5s `check@2x.png` will be used, on Nexus 5 – `check@3x.png`. If there is no image matching the screen density, the closest best option will be selected.
Copy link
Contributor

@davidaurelio davidaurelio Sep 26, 2016

Choose a reason for hiding this comment

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

I’m not a native speaker, but wouldn’t “to the screen density of the device” be even better?

Copy link
Author

Choose a reason for hiding this comment

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

Hey David, it can go both ways. I didn't want to get into changing the wording and grammar.

Copy link
Contributor

Choose a reason for hiding this comment

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

Feel free to go all the way, or keep it like this – your call :-)

@@ -104,7 +104,7 @@ iOS saves multiple sizes for the same image in your Camera Roll, it is very impo

*In the browser* if you don't give a size to an image, the browser is going to render a 0x0 element, download the image, and then render the image based with the correct size. The big issue with this behavior is that your UI is going to jump all around as images load, this makes for a very bad user experience.

*In React Native* this behavior is intentionally not implemented. It is more work for the developer to know the dimensions (or aspect ratio) of the remote image in advance, but we believe that it leads to a better user experience. Static images loaded from the app bundle via the `require('./my-icon.png')` syntax *can be automatically sized* because their dimensions are available immediately at the time of mounting.
*In React Native* this behavior is intentionally not implemented. It is more work for the developer to know the dimensions (or aspect ratio) of the remote image in advance, but we believe that it leads to a better user experience. Static images loaded from the app and bundled via the `require('./my-icon.png')` syntax *can be automatically sized* because their dimensions are available immediately at the time of mounting.
Copy link
Contributor

Choose a reason for hiding this comment

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

This one was actually legit.

@davidaurelio
Copy link
Contributor

Thank you for taking the time to make our documentation better. One of your changes was actually correct. Could you revert that one, please?

@oliviabanis
Copy link
Author

Hey David, thanks for letting me know that one of my changes for actually correct. The other one was correct, too. Take care

@davidaurelio
Copy link
Contributor

davidaurelio commented Sep 26, 2016

@olgabanis oh wow, I’m sorry. That was really misleading. I wanted to say that one of your edits was actually unnecessary – the one that I was commenting on. I just messed up the message, sorry about that.

@ghost ghost 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 Sep 26, 2016
@facebook-github-bot
Copy link
Contributor

It's been a while since the last commit was reviewed and the labels show this pull request needs review. Based on the blame information for the files in this pull request we identified @davidaurelio as a potential reviewer. Could you take a look please or cc someone with more context?

@JoelMarcey
Copy link

cc @lacker @hramos

app and bundled -> app bundle
@lacker
Copy link
Contributor

lacker commented Oct 26, 2016

Hey @olgabanis thanks for sending these fixes! I think I get what David was attempting to say here - the "app bundle" is what that sentence is referring to, using bundle as a noun. So I just tweaked the PR to have the other three changes. Thanks again - I'll ship this now

@lacker
Copy link
Contributor

lacker commented Oct 26, 2016

@facebook-github-bot shipit

@facebook-github-bot facebook-github-bot added GH Review: accepted Import Started This pull request has been imported. This does not imply the PR has been approved. and removed GH Review: review-needed labels Oct 26, 2016
@facebook-github-bot
Copy link
Contributor

@lacker has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@oliviabanis
Copy link
Author

Thanks @lacker and sorry for letting this drag on, I thought it was resolved. Cheers

DanielMSchmidt pushed a commit to DanielMSchmidt/react-native that referenced this pull request Jan 4, 2017
Summary:
Simple documentation errata.
Closes facebook#10098

Differential Revision: D4082724

Pulled By: lacker

fbshipit-source-id: 5b4df66472b37b643ced62fafefe461c0c2c86c7
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. Import Started This pull request has been imported. This does not imply the PR has been approved.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants