Skip to content
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

Merged
merged 7 commits into from
Sep 26, 2017
Merged

[Docs] Add docs reflecting v3 #1121

merged 7 commits into from
Sep 26, 2017

Conversation

lelandrichardson
Copy link
Collaborator

Started working on fixing up the docs for the v3 release

Copy link
Member

@ljharb ljharb left a 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
Copy link
Member

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';
Copy link
Member

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?

Copy link
Collaborator Author

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() });
Copy link
Member

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?

@@ -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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/Enzyme/enzyme/g?

Copy link
Collaborator Author

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?

Copy link
Member

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.

@@ -120,6 +124,8 @@ class Foo extends React.Component {
}
```

TODO: finish talking about this
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lol?

@@ -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.
Copy link
Member

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
Copy link
Member

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

Copy link
Collaborator Author

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good

Copy link
Member

@ljharb ljharb left a 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

@lelandrichardson lelandrichardson merged commit 0c51531 into master Sep 26, 2017
@lelandrichardson lelandrichardson deleted the lmr--v3-docs branch September 26, 2017 06:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants