search

ankane / react-chartkick

Create beautiful JavaScript charts with one line of React
https://chartkick.com/react
MIT License
1.2k stars 58 forks source link

Fixed "Loading..." message on update #16

Closed oncomouse closed 6 years ago

oncomouse commented 6 years ago

This is a possible solution to the problem in #12

The "Loading…" message that appears seems to be hanging around from earlier loads. As the actual React component (rather than the Chartkick object) only needs to update when props.id changes, creating the new Chartkick object in shouldComponentUpdate instead of componentWillUpdate prevents the erroneous loading message from appearing.

ankane commented 6 years ago

Hey @oncomouse, thanks for the PR 👍 Do you have any sample code to reproduce the issue I can try out?

Sexual commented 6 years ago

Can we accept this pull request? It does appear to fix our issue.

oncomouse commented 6 years ago

I somehow missed @ankane's initial comment. Here's a repo that produces the initial issue for me: https://github.com/oncomouse/react-chartkick-example

ankane commented 6 years ago

Hey @oncomouse and @Sexual, can you try the master branch? After reading the React docs, I think componentDidUpdate is the correct hook we want to use.

oncomouse commented 6 years ago

@ankane, the master branch seems to fix this issue. 👍

ankane commented 6 years ago

Great, thanks for confirming. Just pushed a new release.