-
-
Notifications
You must be signed in to change notification settings - Fork 2k
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
[Docs] Add docs reflecting v3 #1121
Conversation
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.
LGTM overall. needs a rebase, and i had some comments
README.md
Outdated
| `enzyme-adapter-react-13` | `^0.13.0` | | ||
|
||
It is possible for the community to create additional (non-official) adapters that will make enzyme | ||
work with other libraries. If you have made one, feel free to make a PR to this README and add a |
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.
should we make a section for this, so people have a place to PR into?
the top level `configure(...)` API. | ||
|
||
```js | ||
import Enzyme from 'enzyme'; |
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.
can we make import { configure } from 'enzyme'
also work, even if it's not in the docs?
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.
@ljharb it does work
import Enzyme from 'enzyme'; | ||
import Adapter from 'enzyme-adapter-react-16'; | ||
|
||
Enzyme.configure({ adapter: new Adapter() }); |
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.
it seems kind of weird that adapters need to be constructed; could adapter
take a constructor instead of an instance perhaps?
docs/guides/migration-from-2-to-3.md
Outdated
@@ -1,7 +1,7 @@ | |||
# Migration Guide for Enzyme v2.x to v3.x | |||
|
|||
The change from Enzyme v2.x to v3.x is a more significant change than in previous major releases, | |||
due to the fact that the internal implementation has been almost completely rewritten. | |||
due to the fact that the internal implementation of Enzyme has been almost completely rewritten. |
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.
s/Enzyme/enzyme/g
?
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.
why? it's capitalized in other places?
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.
Historically we've not capitalized it anywhere; I think that lowercase (to match the repo and npm name) is a better choice.
docs/guides/migration-from-2-to-3.md
Outdated
@@ -120,6 +124,8 @@ class Foo extends React.Component { | |||
} | |||
``` | |||
|
|||
TODO: finish talking about this |
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.
lol?
docs/guides/migration-from-2-to-3.md
Outdated
@@ -242,7 +248,7 @@ wrapper.find('.count').text(); // => "Count: 0" (would have been "Count: 1" in v | |||
|
|||
The problem here is that once we grab the instance using `wrapper.instance()`, Enzyme has no way of | |||
knowing if you are going to execute something that will cause a state transition, and thus does not | |||
know when to ask for an updated render tree from React. As a result, `.text()` never changes value. | |||
know when to ask for an updated render tree from React. As a result, `.text()` never changes value. |
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.
super glad you're upgrading us from typewriter style to computer style 💯
@@ -24,9 +24,3 @@ describe('<Foo />', () => { | |||
}); | |||
|
|||
``` | |||
|
|||
|
|||
## Example Projects |
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.
do we no longer have example projects? or is it not needed because of removing react-compat
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.
IMO these projects were never maintained and have not been updated. if we want to add some to this repo, i think that might help keep them updated... but these are no longer good references, so i've removed them.
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.
Sounds good
0279a9b
to
b5fa2bb
Compare
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 enzyme should be lowercased everywhere, but otherwise this LGTM
b5fa2bb
to
94ec3ce
Compare
Started working on fixing up the docs for the v3 release