-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
chore: prepare docs for Docusaurus v3 upgrade - 2nd iteration #3830
Conversation
✅ Deploy Preview for react-native ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
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.
Ready to review @Simek
@@ -71,7 +71,7 @@ To open the profile in Chrome DevTools: | |||
2. Select the **Performance** tab. | |||
3. Right click and choose **Load profile...** | |||
|
|||
<img src="/docs/assets/openChromeProfile.png" alt="Loading a performance profile on Chrome DevTools" /> | |||
<img src="/docs/assets/openChromeProfile.png" alt="Loading a performance profile on Chrome DevTools" /> |
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.
unwanted extra space change list + image layout
Before: https://reactnative.dev/docs/profile-hermes#open-the-downloaded-profile-in-chrome-devtools
After: https://deploy-preview-3830--react-native.netlify.app/docs/profile-hermes#open-the-downloaded-profile-in-chrome-devtools
- `alert` :boolean | ||
- `badge` :boolean | ||
- `sound` :boolean | ||
- `alert: boolean` | ||
- `badge: boolean` | ||
- `sound: boolean` |
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.
:xyz
, ::xyz
and :::xyz
are "Markdown Directives" and we turn them on by default in Docusaurus v3 to bring admonition support.
This means you should avoid that in your code (unless escaped) otherwise those annotations will magically disappear when upgrading.
I think it's reasonable to make the boolean part of the inline code block
Before: https://reactnative.dev/docs/pushnotificationios#checkpermissions
After: https://deploy-preview-3830--react-native.netlify.app/docs/pushnotificationios#checkpermissions
deprecatedAnimated?: boolean, | ||
deprecatedAnimated?: boolean, |
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.
looks like a typo
Creates an object that represents android theme's default background for selectable elements (?android:attr/selectableItemBackground). `rippleRadius` parameter controls the radius of the ripple effect. | ||
Creates an object that represents android theme's default background for selectable elements (`?android:attr/selectableItemBackground`). `rippleRadius` parameter controls the radius of the ripple effect. |
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.
@@ -160,7 +160,7 @@ React Native renderer leverages structural sharing to minimize the overhead of i | |||
|
|||
In the above example, React creates the new tree using these operations: | |||
|
|||
1. CloneNode(**Node 3**, {backgroundColor: 'yellow'}) → **Node 3'** | |||
1. CloneNode(**Node 3**, `{backgroundColor: 'yellow'}`) → **Node 3'** |
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.
looks reasonable to wrap in code block
{
will mess up with MDX 2 and require to be escaped
[[RCTI18nUtil sharedInstance] allowRTL:YES]; | ||
[[RCTI18nUtil sharedInstance] allowRTL:YES]; | ||
``` | ||
|
||
Android: | ||
|
||
```java | ||
// in MainActivity.java | ||
I18nUtil sharedI18nUtilInstance = I18nUtil.getInstance(); | ||
sharedI18nUtilInstance.allowRTL(context, true); | ||
I18nUtil sharedI18nUtilInstance = I18nUtil.getInstance(); | ||
sharedI18nUtilInstance.allowRTL(context, true); |
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.
<dict> | ||
<key>localhost</key> | ||
<dict> | ||
- <key>NSTemporaryExceptionAllowsInsecureHTTPLoads</key> | ||
+ <key>NSExceptionAllowsInsecureHTTPLoads</key> | ||
<true/> | ||
</dict> | ||
</dict> | ||
<dict> | ||
<key>localhost</key> | ||
<dict> | ||
- <key>NSTemporaryExceptionAllowsInsecureHTTPLoads</key> | ||
+ <key>NSExceptionAllowsInsecureHTTPLoads</key> | ||
<true/> | ||
</dict> | ||
</dict> | ||
[...] | ||
``` | ||
|
||
All we need now is to apply this patch to your source files. While the old `react-native upgrade` process would have prompted you for any small difference, Git is able to merge most of the changes automatically using its 3-way merge algorithm and eventually leave us with familiar conflict delimiters: | ||
|
||
``` | ||
13B07F951A680F5B00A75B9A /* Release */ = { | ||
isa = XCBuildConfiguration; | ||
buildSettings = { | ||
ASSETCATALOG_COMPILER_APPICON_NAME = AppIcon; | ||
13B07F951A680F5B00A75B9A /* Release */ = { | ||
isa = XCBuildConfiguration; | ||
buildSettings = { | ||
ASSETCATALOG_COMPILER_APPICON_NAME = AppIcon; | ||
<<<<<<< ours | ||
CODE_SIGN_IDENTITY = "iPhone Developer"; | ||
FRAMEWORK_SEARCH_PATHS = ( | ||
"$(inherited)", | ||
"$(PROJECT_DIR)/HockeySDK.embeddedframework", | ||
"$(PROJECT_DIR)/HockeySDK-iOS/HockeySDK.embeddedframework", | ||
); | ||
CODE_SIGN_IDENTITY = "iPhone Developer"; | ||
FRAMEWORK_SEARCH_PATHS = ( | ||
"$(inherited)", | ||
"$(PROJECT_DIR)/HockeySDK.embeddedframework", | ||
"$(PROJECT_DIR)/HockeySDK-iOS/HockeySDK.embeddedframework", | ||
); | ||
======= | ||
CURRENT_PROJECT_VERSION = 1; | ||
CURRENT_PROJECT_VERSION = 1; | ||
>>>>>>> theirs | ||
HEADER_SEARCH_PATHS = ( | ||
"$(inherited)", | ||
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/include, | ||
"$(SRCROOT)/../node_modules/react-native/React/**", | ||
"$(SRCROOT)/../node_modules/react-native-code-push/ios/CodePush/**", | ||
); | ||
HEADER_SEARCH_PATHS = ( | ||
"$(inherited)", | ||
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/include, | ||
"$(SRCROOT)/../node_modules/react-native/React/**", | ||
"$(SRCROOT)/../node_modules/react-native-code-push/ios/CodePush/**", | ||
); | ||
``` |
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.
Replace tabs with space for consistency
Avoid this unwanted extra spacing when upgrading:
Before: https://reactnative.dev/blog/2016/12/05/easier-upgrades#how-does-it-work
After: https://deploy-preview-3830--react-native.netlify.app/blog/2016/12/05/easier-upgrades#how-does-it-work
[view setRegion:json ? [RCTConvert MKCoordinateRegion:json] : defaultView.region animated:YES]; | ||
[view setRegion:json ? [RCTConvert MKCoordinateRegion:json] : defaultView.region animated:YES]; |
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.
replace tab with space + reduce indentation for consistency
Avoid this extra spacing when upgrading:
Before: https://reactnative.dev/docs/native-components-ios#events
After: https://deploy-preview-3830--react-native.netlify.app/docs/native-components-ios#events
<> | ||
<img src="/blog/assets/rtl-ama-ios-arabic.png" width={280} style={{ margin: 10 }} /> | ||
|
||
<img src="/blog/assets/rtl-ama-android-hebrew.png" width={280} style={{ margin: 10 }} /> | ||
</> |
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.
Adding a fragment here permits to avoid having an extra useless \n
char when upgrading to MDX v2.
https://twitter.com/badsyntax/status/1695041116893036746
In practice it's not super impactful, but this change will ensure the 2 images look exactly the same before/after the MDX v2 upgrade.
version "1.28.0" | ||
resolved "https://registry.yarnpkg.com/prismjs/-/prismjs-1.28.0.tgz#0d8f561fa0f7cf6ebca901747828b149147044b6" | ||
integrity sha512-8aaXdYvl1F7iC7Xm1spqSaY/OJBpYW3v+KJ+F17iYxvdc8sfjW194COK5wVhMZX45tGteiBQgdvD/nhxcRwylw== | ||
version "1.29.0" |
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.
minor update, the only impact I found in my visual tests was this change in Java code blocks:
Thanks for the care you put into finding these issues and documenting the diff inline to make it easy to review! |
This is a follow-up of #3807
The goal is to adjust docs so that they render exactly the same on MDX v1 and MDX v2.
This permits my visual regression tests to avoid reporting false positives due to MDX behavior changes.
This also permitted to catch a few typos and inconsistencies.
Preview: https://deploy-preview-3830--react-native.netlify.app/