Support configurable update #4362
Conversation
See discussion in the issue for context and possible approaches. When invoking update() inside an event handler, such as onHover, `options.hover.animationDuration` was not being respected. Given that some use cases may require additional animation properties for the manual update call, this commit changes that method signature to accept a configuration object. This object provides backwards compatibility with duration and lazy properties, and also introduces the easing property so that the event animation is different from the global one. Issue #4300
|
@simonbrunel, @etimberg Can you take a look at this approach? 🙏 Also, codeclimate doesn't seem to like the snippet for keeping backwards compatibility. Any suggestions? |
| var me = this; | ||
|
|
||
| if (!config || typeof config !== 'object') { |
There was a problem hiding this comment.
We should have a test, if possible, that verifies that backwards compatibility isn't broken.
There was a problem hiding this comment.
I can try that. Is there any other place other than core.controller.tests.js? Because that isn't testing _bufferedRender, nor render at the moment.
Add tests that guarantee that when update is called manually with arguments, it properly builds the _bufferedRequest or calls render with the proper arguments. It includes test cases for when update is called with legacy arguments (duration and lazy) instead of the config object. Issue #4300
.update() documentation was previously updated but .render() was left out. Since the backwards compatible change was also made to render(), this commit adds documentation for it. Issue #4300
docs/developers/api.md
Outdated
| }) | ||
| ``` | ||
|
|
||
| > **Note:** `.update(duration, lazy)` is still supported in backwards compatibility mode. |
There was a problem hiding this comment.
I would not write about it: it still works but people should not know about it.
docs/developers/api.md
Outdated
| ``` | ||
|
|
||
| > **Note:** `.render(duration, lazy)` is still supported in backwards compatibility mode. |
test/specs/core.controller.tests.js
Outdated
| }); | ||
| }); | ||
|
|
||
| describe('when using backwards compatibility', function() { |
There was a problem hiding this comment.
Can we move that "compatibility" suite in global.deprecations.tests.js as:
describe('Deprecations', function() {
describe('Version 2.7.0', function() {
describe('Chart.Controller.update(duration, lazy)', function() {
// ...
});
describe('Chart.Controller.render(duration, lazy)', function() {
// ...
});
});
describe('Version 2.6.0', function() {
// ...
test/specs/core.controller.tests.js
Outdated
| }); | ||
|
|
||
| describe('when using backwards compatibility', function() { | ||
| describe('when _bufferedRender is true', function() { |
There was a problem hiding this comment.
I don't think we need to differentiate when _bufferedRender is true or false (which is internal, so should not be tested). Both methods (update and render) could be tested by checking that Chart.animationService.addAnimation() is called with the correct arguments?
Instead of relying on internal properties such as _bufferedRender and _bufferedRequest, the tests are now rewritten to focus on the parameters provided when calling Chart.animationService#addAnimation. The mention to the old signature of render and update was also removed from the docs. Issue #4300
|
@simonbrunel Can you take another look? |
| var addAnimationSpy; | ||
|
|
||
| beforeEach(function() { | ||
| chart = acquireChart({ |
There was a problem hiding this comment.
I think it's better to avoid global variables and use this instead:
this.chart = ...
this.addAnimationSpy = ...There was a problem hiding this comment.
Those are declared inside the describe. But I can change them to use this if that's the usual pattern in the project.
| beforeEach(function() { | ||
| chart = acquireChart({ | ||
| type: 'doughnut', | ||
| data: { |
There was a problem hiding this comment.
do we need data for this unit tests?
| }] | ||
| }, | ||
| options: { | ||
| cutoutPercentage: 85, |
There was a problem hiding this comment.
Is this option relevant? we should avoid setting data/options that are not useful for the test
There was a problem hiding this comment.
Both data and cutoutPercentage were set just to have a "consistent" chart instance, so to speak. Since the type is doughnut it made sense to be explicit about the cutout percentage. Will remove them if you think they can cause confusion. 👍
- Use `this` - Do not use properties/data that are not needed for the spec.
| this.addAnimationSpy = spyOn(Chart.animationService, 'addAnimation'); | ||
| }); | ||
|
|
||
| it('adds an animation with the provided options', function() { |
There was a problem hiding this comment.
it('should add... to be consistent with all tests :)
|
Fixed. 👍 |
See discussion in the issue for context and possible approaches.
When invoking
update()inside an event handler, such as onHover,options.hover.animationDurationwas not being respected. Given that some use cases may require additional animation properties for the manual update call, this commit changes that method signature to accept a configuration object.This object provides backwards compatibility with
durationandlazyproperties, and also introduces theeasingproperty so that the event animation is different from the global one.Issue #4300