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

Render a placeholder element, get the container size, and only then render #2

Merged
merged 2 commits into from
May 25, 2016

Conversation

zvictor
Copy link
Contributor

@zvictor zvictor commented May 25, 2016

fix #1

@okonet
Copy link
Owner

okonet commented May 25, 2016

Awesome! I like this approach! Can you please also add test cases and I'd be happy to merge it.

@zvictor
Copy link
Contributor Author

zvictor commented May 25, 2016

all the features are already being tested and passing.

I don't think we should test if we initially rendered a placeholder or not, the same way it was not testing before if a children was being initially rendered with { width: 0, height: 0}

@okonet
Copy link
Owner

okonet commented May 25, 2016

I don't think we should test if we initially rendered a placeholder or not

Why not? I think this is now quite important to test

it was not testing before if a children was being initially rendered with { width: 0, height: 0}

You're right. I missed this one. I've just added such a test in d84fedf

@zvictor
Copy link
Contributor Author

zvictor commented May 25, 2016

how can I check the content of the initial render, before componentDidMount?

@zvictor
Copy link
Contributor Author

zvictor commented May 25, 2016

there you go.
It conflicts with the tests you just created. I haven't fixed because you have to decide if you want to keep the tests you just created or not.

@okonet
Copy link
Owner

okonet commented May 25, 2016

Thanks! I'll merge and take a look at tests myself. Either way I think it's a great improvement over the current state!

@zvictor
Copy link
Contributor Author

zvictor commented May 25, 2016

please let us know at recharts/recharts#105 when you merge it

@okonet okonet merged commit 062378b into okonet:master May 25, 2016
@zvictor
Copy link
Contributor Author

zvictor commented May 30, 2016

Shouldn't we publish a new version on npm?

@okonet
Copy link
Owner

okonet commented May 30, 2016

Will do now. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Initial values
2 participants