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.

Sign up for GitHub

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

Use element options for removeHoverStyle(), even when inheriting #5194

Closed
wants to merge 29 commits into from
Closed

Use element options for removeHoverStyle(), even when inheriting #5194

Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
c98185e
Refactor updateElement and removeHoverStyle
loicbourgois Jan 28, 2018
8de5a31
Update style only
loicbourgois Jan 28, 2018
87929a4
typo
loicbourgois Jan 28, 2018
0bf1910
Refactor
loicbourgois Jan 28, 2018
8f664f1
Revert "Refactor"
loicbourgois Jan 29, 2018
bb2c547
Revert "typo"
loicbourgois Jan 29, 2018
9388dd8
Revert "Update style only"
loicbourgois Jan 29, 2018
ac18953
Revert "Refactor updateElement and removeHoverStyle"
loicbourgois Jan 29, 2018
0f96799
Merge branch 'master' of https://github.com/chartjs/Chart.js into bor…
loicbourgois Jan 29, 2018
a9af225
Implement @simonbrunel solution
loicbourgois Jan 29, 2018
dc2a66e
Mix @simbrunel solution with previous implementation
loicbourgois Jan 29, 2018
af1abd2
Update failing test
loicbourgois Jan 31, 2018
2527620
Remove not.toBe()
loicbourgois Feb 1, 2018
b8c5aef
Merge branch 'master' of https://github.com/chartjs/Chart.js into bor…
loicbourgois Feb 11, 2018
4b30773
Refactor
loicbourgois Feb 11, 2018
c8064d6
Better hover style for line
loicbourgois Feb 11, 2018
fcef4b6
Update radar hover style logic
loicbourgois Feb 11, 2018
fad0561
cleanup
loicbourgois Feb 11, 2018
743a123
refactor
loicbourgois Feb 11, 2018
57f3df0
refactor
loicbourgois Feb 11, 2018
07bf637
refactor
loicbourgois Feb 11, 2018
7448c51
refactor
loicbourgois Feb 11, 2018
0d9a74b
Pass tests
loicbourgois Feb 11, 2018
d89c572
Refactor
loicbourgois Feb 11, 2018
2d9a387
refactor
loicbourgois Feb 11, 2018
ea376e6
setup test
loicbourgois Feb 11, 2018
693fb03
Comment seamingly unecessary code
loicbourgois Feb 11, 2018
92cb840
Setup test for doughnut.controller.removeHoverStyle()
loicbourgois Feb 11, 2018
9cd0686
Test commenting seamingly unnecessary code
loicbourgois Feb 11, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 8 additions & 9 deletions src/controllers/controller.bar.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -468,21 +468,20 @@ module.exports = function(Chart) {
var custom = rectangle.custom || {};
var model = rectangle._model;

rectangle.$previousStyle = {
backgroundColor: model.backgroundColor,
borderColor: model.borderColor,
borderWidth: model.borderWidth
};

model.backgroundColor = custom.hoverBackgroundColor ? custom.hoverBackgroundColor : helpers.valueAtIndexOrDefault(dataset.hoverBackgroundColor, index, helpers.getHoverColor(model.backgroundColor));
model.borderColor = custom.hoverBorderColor ? custom.hoverBorderColor : helpers.valueAtIndexOrDefault(dataset.hoverBorderColor, index, helpers.getHoverColor(model.borderColor));
model.borderWidth = custom.hoverBorderWidth ? custom.hoverBorderWidth : helpers.valueAtIndexOrDefault(dataset.hoverBorderWidth, index, model.borderWidth);
},

removeHoverStyle: function(rectangle) {
var dataset = this.chart.data.datasets[rectangle._datasetIndex];
var index = rectangle._index;
var custom = rectangle.custom || {};
var model = rectangle._model;
var rectangleElementOptions = this.chart.options.elements.rectangle;

model.backgroundColor = custom.backgroundColor ? custom.backgroundColor : helpers.valueAtIndexOrDefault(dataset.backgroundColor, index, rectangleElementOptions.backgroundColor);
model.borderColor = custom.borderColor ? custom.borderColor : helpers.valueAtIndexOrDefault(dataset.borderColor, index, rectangleElementOptions.borderColor);
model.borderWidth = custom.borderWidth ? custom.borderWidth : helpers.valueAtIndexOrDefault(dataset.borderWidth, index, rectangleElementOptions.borderWidth);
helpers.merge(rectangle._model, rectangle.$previousStyle || {});
delete rectangle.$previousStyle;
Copy link
Contributor

Choose a reason for hiding this comment

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

this line also seems like it would be unnecessary. It doesn't hurt to leave it I suppose, but I'm not sure it ever has any effect

}
});

Expand Down
32 changes: 31 additions & 1 deletion test/specs/controller.bar.tests.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1372,13 +1372,23 @@ describe('Chart.controllers.bar', function() {

var meta = chart.getDatasetMeta(1);
var bar = meta.data[0];
var helpers = window.Chart.helpers;

// Change default
chart.options.elements.rectangle.backgroundColor = 'rgb(128, 128, 128)';
chart.options.elements.rectangle.borderColor = 'rgb(15, 15, 15)';
chart.options.elements.rectangle.borderWidth = 3.14;

// Remove to defaults
chart.update();
expect(bar._model.backgroundColor).toBe('rgb(128, 128, 128)');
expect(bar._model.borderColor).toBe('rgb(15, 15, 15)');
expect(bar._model.borderWidth).toBe(3.14);
meta.controller.setHoverStyle(bar);
expect(bar._model.backgroundColor).not.toBe('rgb(128, 128, 128)');
expect(bar._model.borderColor).not.toBe('rgb(15, 15, 15)');
expect(bar._model.backgroundColor).toBe(helpers.getHoverColor('rgb(128, 128, 128)'));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure which one is better :

  • .not.toBe('rgb(128, 128, 128)')
  • .toBe(helpers.getHoverColor('rgb(128, 128, 128)')

Copy link
Member

Choose a reason for hiding this comment

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

I would say toBe is better

expect(bar._model.borderColor).toBe(helpers.getHoverColor('rgb(15, 15, 15)'));
expect(bar._model.borderWidth).toBe(3.14);
meta.controller.removeHoverStyle(bar);
expect(bar._model.backgroundColor).toBe('rgb(128, 128, 128)');
expect(bar._model.borderColor).toBe('rgb(15, 15, 15)');
Expand All @@ -1389,6 +1399,16 @@ describe('Chart.controllers.bar', function() {
chart.data.datasets[1].borderColor = ['rgb(9, 9, 9)', 'rgb(0, 0, 0)'];
chart.data.datasets[1].borderWidth = [2.5, 5];

chart.update();
expect(bar._model.backgroundColor).toBe('rgb(255, 255, 255)');
expect(bar._model.borderColor).toBe('rgb(9, 9, 9)');
expect(bar._model.borderWidth).toBe(2.5);
meta.controller.setHoverStyle(bar);
expect(bar._model.backgroundColor).not.toBe('rgb(255, 255, 255)');
expect(bar._model.borderColor).not.toBe('rgb(9, 9, 9)');
expect(bar._model.backgroundColor).toBe(helpers.getHoverColor('rgb(255, 255, 255)'));
expect(bar._model.borderColor).toBe(helpers.getHoverColor('rgb(9, 9, 9)'));
expect(bar._model.borderWidth).toBe(2.5);
meta.controller.removeHoverStyle(bar);
expect(bar._model.backgroundColor).toBe('rgb(255, 255, 255)');
expect(bar._model.borderColor).toBe('rgb(9, 9, 9)');
Expand All @@ -1401,6 +1421,16 @@ describe('Chart.controllers.bar', function() {
borderWidth: 1.5
};

chart.update();
expect(bar._model.backgroundColor).toBe('rgb(255, 0, 0)');
expect(bar._model.borderColor).toBe('rgb(0, 255, 0)');
expect(bar._model.borderWidth).toBe(1.5);
meta.controller.setHoverStyle(bar);
expect(bar._model.backgroundColor).not.toBe('rgb(255, 0, 0)');
expect(bar._model.borderColor).not.toBe('rgb(0, 255, 0)');
expect(bar._model.backgroundColor).toBe(helpers.getHoverColor('rgb(255, 0, 0)'));
expect(bar._model.borderColor).toBe(helpers.getHoverColor('rgb(0, 255, 0)'));
expect(bar._model.borderWidth).toBe(1.5);
meta.controller.removeHoverStyle(bar);
expect(bar._model.backgroundColor).toBe('rgb(255, 0, 0)');
expect(bar._model.borderColor).toBe('rgb(0, 255, 0)');
Expand Down