-
Notifications
You must be signed in to change notification settings - Fork 24.8k
Added a couple of articles/prepositions that were missing #10098
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
Conversation
By analyzing the blame information on this pull request, we identified @davidaurelio and @JoelMarcey to be potential reviewers. |
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! |
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks! |
@@ -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. |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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. |
There was a problem hiding this comment.
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.
Thank you for taking the time to make our documentation better. One of your changes was actually correct. Could you revert that one, please? |
Hey David, thanks for letting me know that one of my changes for actually correct. The other one was correct, too. Take care |
@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. |
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? |
app and bundled -> app bundle
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 |
@facebook-github-bot shipit |
@lacker has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
Thanks @lacker and sorry for letting this drag on, I thought it was resolved. Cheers |
Summary: Simple documentation errata. Closes facebook#10098 Differential Revision: D4082724 Pulled By: lacker fbshipit-source-id: 5b4df66472b37b643ced62fafefe461c0c2c86c7
Simple documentation errata.