Upgrade React-Native website to Docusaurus v3 #3780
Conversation
✅ Deploy Preview for react-native ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
This comment was marked as outdated.
This comment was marked as outdated.
slorber
changed the title
There was a problem hiding this comment.
Hi @slorber, thank you so much for this PR. I left a few comments, mainly for clarification.
The only significant piece I am not confident to review are the Snack updates: at first glance, they looks good to me, but let's wait for another pair of eyes to double check them.
| if (!ExecutionEnvironment.canUseDOM) { | ||
| const isBrowser = useIsBrowser(); | ||
| if (!isBrowser) { |
There was a problem hiding this comment.
This fixes a little React hydration issue. The issue was already there, and is not a blocker, but is now logged thanks to React 18 new hydration callback onRecoverableError(error) (https://react.dev/blog/2022/03/29/react-v18)
The SSR/SSG render and the first CSR render outputs should match, so things such as typeof window !== "undefined" etc should be avoided.
Thanks @cipolleschi Regarding the SnackPlayer, unless you know well the differences between MDX v1/v2 it would be hard to review technically. IMHO you'd rather focus on the outcome: is the snack player still working on all the pages in the deploy preview, or do you see any regression? |
Not really, I was just mentioning it in case someone among @cortinico @TheSavior or @Simek wants to double-check that code! :D |
website/src/css/customTheme.scss
Outdated
| */ | ||
| figcaption > p:last-child { | ||
| margin-bottom: 0; | ||
| } |
There was a problem hiding this comment.
This needs a new line at the end of the file it seems
There was a problem hiding this comment.
I added a new line but not sure why it's needed? CI is passing. Is there linter I don't see?
There was a problem hiding this comment.
I mentioned it because GitHub showed this icon presumably because there wasn't an extra new line, whereas all other files have that extra new line.
And yeah, it's normally enforced with a linter. See react-native's main eslint config: https://github.com/facebook/react-native/blob/2be409ff55b50563a5f123f7823fa7cdb72dbef9/packages/eslint-config-react-native/index.js#L264
| @@ -94,7 +94,7 @@ return <S3Album track/> | |||
|
|
|||
| AWS Amplify favors a convention over configuration style of development, with a global initialization routine or initialization at the category level. The quickest way to get started is with an [aws-exports file](https://aws.amazon.com/blogs/mobile/enhanced-javascript-development-with-aws-mobile-hub/). However, developers can also use the library independently with existing resources. | |||
|
|
|||
| For a deep dive into the philosophy and to see a full demo, check out the video from [AWS re:Invent](https://www.youtube.com/watch?v=vAjf3lyjf8c). | |||
| For a deep dive into the philosophy and to see a full demo, check out the video from [AWS re\:Invent](https://www.youtube.com/watch?v=vAjf3lyjf8c). | |||
There was a problem hiding this comment.
Why does this need to be escaped?
There was a problem hiding this comment.
Docusaurus v3 now uses https://github.com/remarkjs/remark-directive to provide support for admonitions, and other things using syntax such as :textDirective, ::leafDirective and :::containerDirective.
Re:invent is parsed as Re + :invent text directive.
This is what happens in the MDX playground if you turn the remark-directive option on:
This regression was captured by the visual regression tests:
https://app.argos-ci.com/slorber/rnw-visual-tests/builds/32/56012838
Technically it should probably be possible to add some code in Docusaurus v3 so that this escaping becomes un-necessary, and the unhandled AST directive nodes get serialized back as raw text. For now it's not implemented, and not even sure who should implement that 😅. Will think about it. I hope in the meantime it's not a blocker for you to merge this PR.
| className="btn"> | ||
| Read more | ||
| </a> | ||
| className="btn">Read more</a> |
There was a problem hiding this comment.
Was this change made by a formatter or manually?
There was a problem hiding this comment.
These changes are manual and intentional.
With MDX v2, using new lines now create extra paragraphs, cf the MDX playground:
And RN website adds padding-bottom to all paragraphs of the website, so this extra <p> tag visually changes the way this is rendered.
I'm not sure the intent is to create a <a><p>Read more</p></a> here so refactored to avoid that and keep <a>Read more</a> as the final output.
There are only 4 similar cases and they are only found in very old blog posts, so I thought it was better to adjust those 4 blog posts rather than fixing it with CSS.
Somehow it's the exact same case as the <figcaption> case that you commented here: #3780 (comment)
But there are much more recent multiline usage cases of <figcaption> on the RNW site, like this one:
<figure>
<img src="/docs/assets/d_pressable_anatomy.svg" width="1000" alt="Diagram of HitRect and PressRect and how they work." />
<figcaption>
You can set <code>HitRect</code> with <code>hitSlop</code> and set <code>PressRect</code> with <code>pressRetentionOffset</code>.
</figcaption>
</figure>That's the reason I decided to use CSS for <figcaption> instead of refactoring many MDX docs:
/*
Prevent useless bottom margin for multiline <figcaption> tags in MDX:
<figcaption>
some paragraph text
</figcaption>
*/
figcaption > p:last-child {
margin-bottom: 0;
}
|
Thanks for the review @cipolleschi @TheSavior Let me know if I can do anything else ;) |
|
🎊 thanks 🎉 🥳 |
WIP PR to upgrade to Docusaurus v3 canary
I will let you know when it's ready to review
Deploy preview: https://deploy-preview-3780--react-native.netlify.app/
Argos visual tests
Note to myself: we can revert usage of
｜in favor of\|, see change made in #3807 (comment)Follow-up of: