Skip to content

Conversation

@simonbrunel
Copy link
Member

@simonbrunel simonbrunel commented Oct 1, 2016

In order to simulate real-time chart updates (i.e. horizontal animation), it's necessary to distinguish a removed or added value from a simple update. The dataset controller now hooks array methods that alter the data array length to synchronize metadata accordingly. Also remove the duplicate calls of updateBezierControlPoints() for line and radar charts.

Examples: http://playground.abysscorp.org/chartjs/livecharts/
Plunker: http://plnkr.co/Imxwl9OQJuaMepLNy6ly
Related issues: #1997 #3126

Copy link
Member

@etimberg etimberg left a comment

Choose a reason for hiding this comment

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

These look good. just one minor comment about the comments

},

/**
* private
Copy link
Member

Choose a reason for hiding this comment

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

should this be "@Private" ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed :)

@simonbrunel simonbrunel force-pushed the live-chart branch 2 times, most recently from 35009b7 to dae14e8 Compare October 1, 2016 16:30
@etimberg
Copy link
Member

etimberg commented Oct 1, 2016

I am 👍 to merge this. I don't know if you prefer waiting for any other comments or not

In order to simulate real-time chart updates (i.e. horizontal animation), it's necessary to distinguish a removed or added value from a simple update. The dataset controller now hooks array methods that alter the data array length to synchronize metadata accordingly. Also remove the duplicate calls of updateBezierControlPoints() for line and radar charts.
@simonbrunel
Copy link
Member Author

Added data cleanup (datasetController.destroy() method) and associated tests:

image

Copy link
Member

@etimberg etimberg left a comment

Choose a reason for hiding this comment

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

Just the one comment on documentation. Other than that, this is good to go 👍

}
},

createMetaDataset: function() {
Copy link
Member

Choose a reason for hiding this comment

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

I realized this method isn't mentioned in the docs ... is that intentional? We don't document the datasetElementType and dataElementType properties either

Copy link
Member Author

Choose a reason for hiding this comment

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

You right, we should document datasetElementType and dataElementType, not sure about createMetaDataset and createMetaData. That can be done in a future PR though :)

Copy link
Member

Choose a reason for hiding this comment

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

ok, wll merge this

@etimberg
Copy link
Member

etimberg commented Oct 3, 2016

Can we merge this?

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.

2 participants