-
Notifications
You must be signed in to change notification settings - Fork 927
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
Resolves Issue #521 Draft #541
Conversation
…er vs ReactDOM.hydrate
…hydrate used when toggling clientOnly parameter in ReactComponent
Thanks for the PR. Please check the build failures by clicking the red X on the latest commit. Once the build passes let me know and I’ll give it some review :) |
Hi there @suhailnaw, do you have time to continue with this PR? If not I'll finish it up and merge it in. |
I was having difficulty building locally on my Mac. Can I use NET Core for my first build using the dotnet build command? |
Hey @dustinsoftware, I got the build to pass! Do you have any feedback on this PR? |
Looks good at a first glance. Need to pull down and test locally before merging. |
I had to bump React to a version higher than 16.0 to get the bug to repro. Thanks for the PR! |
Thanks @suhailnaw for submitting this pull request, and thanks @dustinsoftware for reviewing and merging it! |
Thank you for all your support @dustinsoftware! Feels good to have an open source contribution under my belt :) |
@suhailnaw this has shipped, here are the release notes https://reactjs.net/2018/06/3.4.0-release.html |
Awesome, thanks for the shoutout on the release notes @dustinsoftware, I really appreciate it!! |
I wrote the release notes 😄 Thanks for your pull request! |
Thanks @Daniel15!! :) |
Hi, I wanted to submit my first open source PR for issue #521 for some review!
I believe I set this correctly in ReactEnvironment.cs on lines 305 and 307. Since this is a new parameter for ReactComponent, do I need to go in and add the ClientOnly parameter to every call of ReactComponent? Or should I set a default value?
I used a ternary operator for the conditional in ReactComponent.cs on line 236. I saw ternary operators used in other parts of the codebase, is it appropriate to use here? Also, am I correctly referencing the ClientOnly property from the ReactComponent?
I added two unit tests, one for each case.