-
Notifications
You must be signed in to change notification settings - Fork 7.7k
Update docs for stable Hooks #1629
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Conversation
This reverts commit e301239.
…into gaearon-hooks-stable
Deploy preview for reactjs ready! Built with commit 3091413 |
Also need Sunil’s change into the log. “Add TestUtils.act() to match browser behavior in tests” or something. |
(And changelog updates need to be reflected in React PR) |
That's already in the blog post?
|
Deploy preview for reactjs ready! Built with commit 8fa112e |
}); | ||
|
||
it('can render and update a counter', () => { | ||
// Test first render and componentDidMount |
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.
Oops this shouldn’t mention cDM
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.
Meaning, it should be “first render and effect”
expect(label.textContent).toBe('You clicked 0 times'); | ||
expect(document.title).toBe('You clicked 0 times'); | ||
|
||
// Test second render and componentDidUpdate |
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.
Same
content/docs/hooks-faq.md
Outdated
}); | ||
|
||
it('can render and update a counter', () => { | ||
// Test first render and componentDidMount |
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.
Same
content/docs/hooks-faq.md
Outdated
expect(label.textContent).toBe('You clicked 0 times'); | ||
expect(document.title).toBe('You clicked 0 times'); | ||
|
||
// Test second render and componentDidUpdate |
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.
Same
I mean Changelog section in the end which should match the Changelog PR in React repo and lists all important changes. We haven’t added Sunil’s change there but we should. It’s a new API. |
Hm. I guess it seems really redundant since it's mentioned so explicitly right above, but sure I'll sync whatever Andrew adds to the CHANGELOG once he's added it. |
By that logic we could also remove Hooks PR from it since Hooks are also mentioned :-) We generally leave blog changelog consistent with what’s on GH release and in Changelog.md in the repo. It’s more of a detailed look if you want to see which PR implemented what, and the discussion. Or if you got breakage and want to see the exact list of changes. Whereas body of blog post is a friendly summary. |
That's fine. I wasn't arguing against it, just saying it seemed redundant. I already pushed the update, as you'll see above 😄 |
Yeah I just wanted to explain why. It’s intentionally redundant. :-) I forgot this but we should probably also add TestRenderer.act() to the reference. Maybe just “Same as TestUtils.act() but for the test renderer” with an explanation link. Not urgent tho because RN doesn’t have hooks yet anyway. |
Fork of #1593 with release date modified.