-
-
Notifications
You must be signed in to change notification settings - Fork 250
Update React page #593
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
Update React page #593
Conversation
@fhammerschmidt @cknitt would you take a look? |
let make = (~value, ~children) => { | ||
React.createElement(provider, {"value": value, "children": children}) | ||
} | ||
let make = React.Context.provider(context) |
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.
type props
is missing
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 think we don't need to add the type props
for context. It seems inferred properly with https://github.com/rescript-lang/rescript-react/blob/2c5dea342645921b3efe8b47022fe1d73012ad6e/src/React.res#L82
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.
Ah, great, didn't know that, I thought one always had to define a type props
in the module like one always had to define a function makeProps
in V3. 👍
let context = React.createContext(() => ()) | ||
|
||
module Provider = { | ||
let make = React.Context.provider(context) |
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.
type props
is missing
Once the review is done, I'm going to rebase to master for resolving the conflicts. |
Co-authored-by: Christoph Knittel <[email protected]>
Co-authored-by: Christoph Knittel <[email protected]>
Co-authored-by: Christoph Knittel <[email protected]>
Co-authored-by: Christoph Knittel <[email protected]>
Co-authored-by: Christoph Knittel <[email protected]>
Co-authored-by: Christoph Knittel <[email protected]>
Rebased to upstream master |
@mununki is this ready to merge, or are there outstanding issues? |
Yes This PR is ready. Does it look okay? |
I'm going ahead and merge. It's still possible to fine tune if there are additional comments later. |
#592