refactor: switch to React Router #1115
Conversation
jderochervlk
force-pushed
the
vlk-v12-react-router
branch
from
3d98702 to
71c2897
Compare
Deploying rescript-lang-org with
|
| Latest commit: |
f20ebd5
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://fafaed2f.rescript-lang.pages.dev |
| Branch Preview URL: | https://vlk-v12-react-router.rescript-lang.pages.dev |
…ript-lang/rescript-lang.org into vlk-v12-react-router
…ript-lang/rescript-lang.org into vlk-v12-react-router
…nto vlk-v12-react-router
|
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
| @@ -0,0 +1,17 @@ | |||
| let loader = async () => { | |||
| let props = await Packages.getStaticProps() | |||
There was a problem hiding this comment.
Nit: this binding is a bit unnecessary
There was a problem hiding this comment.
I tried moving it out of the Packages file, but there are a lot of dependencies on other things in that file.
|
|
||
| - Make sure to give a descriptive package name. We usually use `rescript-[name-of-js-lib]` for packages that bind to a specific JS library on npm. | ||
| - Use names that are self explanatory (no weird marketing terms / fantasy words if possible). | ||
| - Use names that are self explanatory (no weird marketing terms /pages/docs/guidelines fantasy words if possible). |
There was a problem hiding this comment.
Probably. Fixed.
| - [bun](https://bun.sh/) | ||
| - [deno](http://deno.com/) | ||
| - Configure `"nodeModulesDir": "auto"` in `deno.json` | ||
| <div class="install-list"> |
There was a problem hiding this comment.
Is this temporary or why is it needed?
There was a problem hiding this comment.
There are some styles that need to be applied otherwise the list looks wrong.
.install-list {
li {
ul {
@apply !mb-0;
}
}
}
This is caused by switching from one markdown tool to another.
src/bindings/Mdast.res
Outdated
| let child = list.children->Option.flatMap(children => children[0]) | ||
| switch (list.url, child) { | ||
| | (Some(url), Some(child)) => { | ||
| if child.value->String.includes("title:") { |
There was a problem hiding this comment.
Yup, this was solved somewhere else and is no longer needed. Fixed.
| @@ -576,6 +576,7 @@ let useCompilerManager = ( | |||
| } | |||
| ) | |||
| | Executing({state, jsCode}) => | |||
| () | |||
src/components/CodeMirror.res
Outdated
| @@ -546,14 +550,22 @@ let make = // props relevant for the react wrapper | |||
| ~keyMap=KeyMap.Default, | |||
| ~lineWrapping=false, | |||
| ): React.element => { | |||
| Console.debug("staring codemirror") | |||
There was a problem hiding this comment.
if not then it should be
"starting codemirror"
src/components/README.md
Outdated
| @@ -0,0 +1,162 @@ | |||
| # rescript-lang.org | |||
There was a problem hiding this comment.
why is this file in src/components now?
There was a problem hiding this comment.
Not sure, but I removed the duplicate.
src/ApiDocs.res
Outdated
|
|
||
| let toctree = tree->Dict.get(moduleName) | ||
| Ok({module_, toctree: Obj.magic({name: "root", path: [], children: []})}) | ||
| // let toctree = tree->Dict.get(moduleName) |
src/BlogLoader.res
Outdated
| // The path isn't included when we are rending a post vs listing them | ||
| let archived = try { | ||
| mdx.path->String.includes("/archived/") | ||
| } catch { |
There was a problem hiding this comment.
is this needed? String.includes usually only throws when you give it a regex?
There was a problem hiding this comment.
I updated the type so this is no longer needed.
| @@ -0,0 +1,177 @@ | |||
| type t = [ | |||
src/Playground.res
Outdated
| require("plugins/cm-reason-mode"); | ||
| } | ||
| `) | ||
| // CodeMirror 6 - no longer need these requires since we import directly in the component |
There was a problem hiding this comment.
I guess we can just remove this comment then.
src/Playground.res
Outdated
| ->Option.getOr(CodeMirror.KeyMap.Default) | ||
| CodeMirror.KeyMap.Default | ||
|
|
||
| // Dom.Storage2.localStorage |
src/Redirects.res
Outdated
| @@ -0,0 +1 @@ | |||
|
|
|||
src/TableOfContents.res
Outdated
| entries: [], | ||
| } | ||
|
|
||
| type c = { |
There was a problem hiding this comment.
what's c? content? Name could be a bit more descriptive.
There was a problem hiding this comment.
Renamed it to context.
mdx-manifest.json
Outdated
| @@ -0,0 +1,1332 @@ | |||
| [ | |||
| { | |||
| "path": "/home/josh/Dev/rescript-lang.org/_blogposts/2025-04-11-introducing-unified-operators.mdx", | |||
There was a problem hiding this comment.
This file shouldn't be commited. I added it to .gitignore
|
Okay done with the full review. I saw a lot of TODOs still that I think should be addressed before the merge, but I only commented where I saw an Issue and there was no TODO already. |
Uh oh!
There was an error while loading. Please reload this page.