Add Switch and Link components #6
Conversation
README.md
Outdated
| npm install --save bs-react-router | ||
| ``` | ||
|
|
||
| Then add bs-react-router to bs-dependencies in your bsconfig.json: |
There was a problem hiding this comment.
Nitpick: formatting with backticks
Add `bs-react-router` to your `bs-dependencies`: **bsconfig.json**
| } | ||
| ``` | ||
|
|
||
| ## Example |
There was a problem hiding this comment.
Suggestion: we can move the examples to actual *.re files, you know for sure then if it compiles ;)
E.g. https://github.com/reasonml-community/bs-downshift/tree/master/examples
There was a problem hiding this comment.
Yes this is a very good point ! Good catch !
package.json
Outdated
| @@ -1,6 +1,6 @@ | |||
| { | |||
| "name": "bs-react-router", | |||
| "version": "1.1.1", | |||
| "version": "1.1.2", | |||
There was a problem hiding this comment.
- I would avoid bumping the version in a PR, it should be done separately
- it should be a
minorbump, not apatch
There was a problem hiding this comment.
Definitely !! @bassjacob this is not a patch, this is a minor version cause of this change from this comment #6 (comment)
package.json
Outdated
| "react-router": "^4.1.1", | ||
| "react-router-dom": "^4.1.1", | ||
| "react-router": "^4.2.0", | ||
| "react-router-dom": "^4.2.2", |
src/reactRouter.re
Outdated
| [@bs.module "react-router-dom"] external _switch : ReasonReact.reactClass = "Switch"; | ||
| let make = (children) => | ||
| ReasonReact.wrapJsForReason( | ||
| ~reactClass=_switch, |
There was a problem hiding this comment.
Nitpick: you can simply name the external reactClass, then you can avoid passing the variable
[@bs.module "react-router-dom"] external reactClass : ReasonReact.reactClass = "Switch";
ReasonReact.wrapJsForReason(~reactClass, ~props=Js.Obj.empty(), children);
| [@bs.module "react-router-dom"] external link : ReasonReact.reactClass = "Link"; | ||
| let make = | ||
| ( | ||
| ~_to: string, |
There was a problem hiding this comment.
Should we use _to or to_? Do you know if there is a convention for such cases?
There was a problem hiding this comment.
That was a good question ! My guess is to_ because _to looks like a non-used variable so I change uses.
|
Hello! Sorry for the churn, but we've made a router today, so I'm planning to move this into the reasonml-old org. Though maybe some of you would like to maintain it? |
examples/reason_react_router.re
Outdated
|
|
||
| <Switch> | ||
| <Route path="/" exact=true component=(() => <HomePage />) /> | ||
| <Route path="/user" component=(() => <UserPage />) /> |
There was a problem hiding this comment.
Suggestion: provide a comment that explain why you wrap the component into a function.
There was a problem hiding this comment.
Also be careful that if you keep it like this the UserPage component won't receive the router props (match, location, ...).
However we could do a little trick:
module UserPage = {
let component = ...;
let make = ...;
};
let userPageComponent =
UserPage.(
ReasonReact.wrapReasonForJs(~component, jsProps =>
make(~match=jsProps##_match, ~location=jsProps##location, [||])
)
);
<Route component=userPageComponent />There was a problem hiding this comment.
@emmenko The wrapReasonForJs returns a type of ReactClass and the component attributes of the router requires a ReactElement type. Should I use this method or transform another way ?
For the moment, I just fixed some comments you advised rebase on master.
There was a problem hiding this comment.
Ah right. Actually I think it's easier to use ReactClass (I also use it in one of my projects).
~component: option(ReasonReact.reactClass)=?,
@bassjacob I have added some components