From 051fdadc6f010e44ea8b2e6ae5443bf9fe882d1d Mon Sep 17 00:00:00 2001 From: xavijam Date: Tue, 28 Feb 2017 15:18:03 +0100 Subject: [PATCH 1/4] Fixed problem with Bubbles legend when a new analysis is applied --- .../data/legends/legend-bubble-definition-model.js | 10 ++++++---- .../legends/legend-bubble-definition-model.spec.js | 11 +++++++++++ 2 files changed, 17 insertions(+), 4 deletions(-) diff --git a/lib/assets/core/javascripts/cartodb3/data/legends/legend-bubble-definition-model.js b/lib/assets/core/javascripts/cartodb3/data/legends/legend-bubble-definition-model.js index 12a007b89c..c0126936a3 100755 --- a/lib/assets/core/javascripts/cartodb3/data/legends/legend-bubble-definition-model.js +++ b/lib/assets/core/javascripts/cartodb3/data/legends/legend-bubble-definition-model.js @@ -1,6 +1,7 @@ var _ = require('underscore'); var LegendBaseDefModel = require('./legend-base-definition-model'); var LegendColorHelper = require('../../editor/layers/layer-content-views/legend/form/legend-color-helper'); +var DEFAULT_BUBBLES_COLOR = '#999999'; module.exports = LegendBaseDefModel.extend({ defaults: _.extend({}, LegendBaseDefModel.prototype.defaults, @@ -38,8 +39,9 @@ module.exports = LegendBaseDefModel.extend({ }, _onChangeStyle: function () { - if (!this.layerDefinitionModel.get('autoStyle')) { - this.set('fillColor', this._inheritStyleColor(this.layerDefinitionModel.styleModel)); + var styleModel = this.layerDefinitionModel.styleModel; + if (!this.layerDefinitionModel.get('autoStyle') && !styleModel.hasNoneStyles()) { + this.set('fillColor', this._inheritStyleColor(styleModel)); } }, @@ -69,7 +71,7 @@ module.exports = LegendBaseDefModel.extend({ _inheritStyleColor: function (styleModel) { var fill = styleModel.get('fill'); var stroke = styleModel.get('stroke'); - var color = fill.color || stroke.color; - return LegendColorHelper.getBubbles(color).color.fixed; + var color = (fill && fill.color) || (stroke && stroke.color); + return (color && LegendColorHelper.getBubbles(color).color.fixed) || DEFAULT_BUBBLES_COLOR; } }); diff --git a/lib/assets/core/test/spec/cartodb3/data/legends/legend-bubble-definition-model.spec.js b/lib/assets/core/test/spec/cartodb3/data/legends/legend-bubble-definition-model.spec.js index abbea55055..cb280c1505 100644 --- a/lib/assets/core/test/spec/cartodb3/data/legends/legend-bubble-definition-model.spec.js +++ b/lib/assets/core/test/spec/cartodb3/data/legends/legend-bubble-definition-model.spec.js @@ -18,6 +18,9 @@ describe('data/legends/legend-bubble-defintion-model', function () { }); style = new Backbone.Model(); + style.hasNoneStyles = function () { + return false; + }; layerDef1.styleModel = style; this.model = new LegendDefinitionModel(null, { @@ -34,6 +37,14 @@ describe('data/legends/legend-bubble-defintion-model', function () { style.trigger('style:update'); expect(this.model._inheritStyleColor).toHaveBeenCalled(); }); + + it('should not _inheritStyleColor if autostyle is none type', function () { + style.hasNoneStyles = function () { + return true; + }; + style.trigger('style:update'); + expect(this.model._inheritStyleColor).not.toHaveBeenCalled(); + }); }); describe('with autostyle', function () { From 6318bc0d4f1d1d3e834b00bf61977964a69d2f15 Mon Sep 17 00:00:00 2001 From: xavijam Date: Wed, 1 Mar 2017 09:42:20 +0100 Subject: [PATCH 2/4] Fixing problems with bubble legend when styles have changed --- .../data/legends/legend-bubble-definition-model.js | 9 +++++++-- .../data/legends/legend-bubble-definition-model.spec.js | 5 ++++- 2 files changed, 11 insertions(+), 3 deletions(-) diff --git a/lib/assets/core/javascripts/cartodb3/data/legends/legend-bubble-definition-model.js b/lib/assets/core/javascripts/cartodb3/data/legends/legend-bubble-definition-model.js index c0126936a3..468d16b470 100755 --- a/lib/assets/core/javascripts/cartodb3/data/legends/legend-bubble-definition-model.js +++ b/lib/assets/core/javascripts/cartodb3/data/legends/legend-bubble-definition-model.js @@ -41,7 +41,7 @@ module.exports = LegendBaseDefModel.extend({ _onChangeStyle: function () { var styleModel = this.layerDefinitionModel.styleModel; if (!this.layerDefinitionModel.get('autoStyle') && !styleModel.hasNoneStyles()) { - this.set('fillColor', this._inheritStyleColor(styleModel)); + this.save({ fillColor: this._inheritStyleColor(styleModel) }); } }, @@ -72,6 +72,11 @@ module.exports = LegendBaseDefModel.extend({ var fill = styleModel.get('fill'); var stroke = styleModel.get('stroke'); var color = (fill && fill.color) || (stroke && stroke.color); - return (color && LegendColorHelper.getBubbles(color).color.fixed) || DEFAULT_BUBBLES_COLOR; + + if (color && (color.fixed || color.range)) { + return LegendColorHelper.getBubbles(color).color.fixed; + } else { + return DEFAULT_BUBBLES_COLOR; + } } }); diff --git a/lib/assets/core/test/spec/cartodb3/data/legends/legend-bubble-definition-model.spec.js b/lib/assets/core/test/spec/cartodb3/data/legends/legend-bubble-definition-model.spec.js index cb280c1505..9359efc8cc 100644 --- a/lib/assets/core/test/spec/cartodb3/data/legends/legend-bubble-definition-model.spec.js +++ b/lib/assets/core/test/spec/cartodb3/data/legends/legend-bubble-definition-model.spec.js @@ -30,12 +30,14 @@ describe('data/legends/legend-bubble-defintion-model', function () { }); spyOn(this.model, '_inheritStyleColor'); + spyOn(this.model, 'save'); }); describe('without autostyle', function () { it('should _inheritStyleColor on custom autostyle', function () { style.trigger('style:update'); expect(this.model._inheritStyleColor).toHaveBeenCalled(); + expect(this.model.save).toHaveBeenCalled(); }); it('should not _inheritStyleColor if autostyle is none type', function () { @@ -44,6 +46,7 @@ describe('data/legends/legend-bubble-defintion-model', function () { }; style.trigger('style:update'); expect(this.model._inheritStyleColor).not.toHaveBeenCalled(); + expect(this.model.save).not.toHaveBeenCalled(); }); }); @@ -52,7 +55,7 @@ describe('data/legends/legend-bubble-defintion-model', function () { layerDef1.set('autoStyle', 'wadus'); }); - it('should _inheritStyleColor on custom autostyle', function () { + it('should not _inheritStyleColor on custom autostyle', function () { style.trigger('style:update'); expect(this.model._inheritStyleColor).not.toHaveBeenCalled(); }); From 9fc0346c341c967cbb973bf9660e3e105eb29fd7 Mon Sep 17 00:00:00 2001 From: xavijam Date: Wed, 1 Mar 2017 10:55:53 +0100 Subject: [PATCH 3/4] Take color when bubble legend is created or updated --- .../legends/legend-bubble-definition-model.js | 19 +------------ .../legend-manager.js | 13 ++++++++- .../legend-bubble-definition-model.spec.js | 28 ------------------- .../legend-manager.spec.js | 18 ++++++------ 4 files changed, 22 insertions(+), 56 deletions(-) diff --git a/lib/assets/core/javascripts/cartodb3/data/legends/legend-bubble-definition-model.js b/lib/assets/core/javascripts/cartodb3/data/legends/legend-bubble-definition-model.js index 468d16b470..dec2da2b23 100755 --- a/lib/assets/core/javascripts/cartodb3/data/legends/legend-bubble-definition-model.js +++ b/lib/assets/core/javascripts/cartodb3/data/legends/legend-bubble-definition-model.js @@ -7,17 +7,12 @@ module.exports = LegendBaseDefModel.extend({ defaults: _.extend({}, LegendBaseDefModel.prototype.defaults, { type: 'bubble', - fillColor: null, + fillColor: DEFAULT_BUBBLES_COLOR, topLabel: '', bottomLabel: '' } ), - initialize: function (attrs, opts) { - LegendBaseDefModel.prototype.initialize.call(this, attrs, opts); - this._initBinds(); - }, - parse: function (r, opts) { var attrs = LegendBaseDefModel.prototype.parse.call(this, r); @@ -33,18 +28,6 @@ module.exports = LegendBaseDefModel.extend({ return attrs; }, - _initBinds: function () { - this.listenTo(this.layerDefinitionModel, 'change:cartocss', this._onChangeStyle.bind(this)); - this.listenTo(this.layerDefinitionModel.styleModel, 'style:update', this._onChangeStyle.bind(this)); - }, - - _onChangeStyle: function () { - var styleModel = this.layerDefinitionModel.styleModel; - if (!this.layerDefinitionModel.get('autoStyle') && !styleModel.hasNoneStyles()) { - this.save({ fillColor: this._inheritStyleColor(styleModel) }); - } - }, - toJSON: function () { return _.extend( {}, diff --git a/lib/assets/core/javascripts/cartodb3/deep-insights-integration/legend-manager.js b/lib/assets/core/javascripts/cartodb3/deep-insights-integration/legend-manager.js index 9263cfe718..1cb8bae579 100755 --- a/lib/assets/core/javascripts/cartodb3/deep-insights-integration/legend-manager.js +++ b/lib/assets/core/javascripts/cartodb3/deep-insights-integration/legend-manager.js @@ -1,5 +1,7 @@ var _ = require('underscore'); var LegendFactory = require('../editor/layers/layer-content-views/legend/legend-factory'); +var LegendColorHelper = require('../editor/layers/layer-content-views/legend/form/legend-color-helper'); +var DEFAULT_BUBBLES_COLOR = '#999999'; var onChange = function (layerDefModel) { var styleModel = layerDefModel.styleModel; @@ -62,7 +64,16 @@ var onChange = function (layerDefModel) { if (LegendFactory.isEnabledType('size')) { if (size && size.attribute !== undefined) { - LegendFactory.createLegend(layerDefModel, 'bubble', {title: size.attribute}); + var attrs = { title: size.attribute }; + + if (!isAutoStyleApplied) { + // Do not apply the fill color of an autostyle ramp + attrs.fillColor = color && (color.fixed || color.range) + ? LegendColorHelper.getBubbles(color).color.fixed + : DEFAULT_BUBBLES_COLOR; + } + + LegendFactory.createLegend(layerDefModel, 'bubble', attrs); } else { LegendFactory.removeLegend(layerDefModel, 'bubble'); } diff --git a/lib/assets/core/test/spec/cartodb3/data/legends/legend-bubble-definition-model.spec.js b/lib/assets/core/test/spec/cartodb3/data/legends/legend-bubble-definition-model.spec.js index 9359efc8cc..3e92ca8c63 100644 --- a/lib/assets/core/test/spec/cartodb3/data/legends/legend-bubble-definition-model.spec.js +++ b/lib/assets/core/test/spec/cartodb3/data/legends/legend-bubble-definition-model.spec.js @@ -32,32 +32,4 @@ describe('data/legends/legend-bubble-defintion-model', function () { spyOn(this.model, '_inheritStyleColor'); spyOn(this.model, 'save'); }); - - describe('without autostyle', function () { - it('should _inheritStyleColor on custom autostyle', function () { - style.trigger('style:update'); - expect(this.model._inheritStyleColor).toHaveBeenCalled(); - expect(this.model.save).toHaveBeenCalled(); - }); - - it('should not _inheritStyleColor if autostyle is none type', function () { - style.hasNoneStyles = function () { - return true; - }; - style.trigger('style:update'); - expect(this.model._inheritStyleColor).not.toHaveBeenCalled(); - expect(this.model.save).not.toHaveBeenCalled(); - }); - }); - - describe('with autostyle', function () { - beforeEach(function () { - layerDef1.set('autoStyle', 'wadus'); - }); - - it('should not _inheritStyleColor on custom autostyle', function () { - style.trigger('style:update'); - expect(this.model._inheritStyleColor).not.toHaveBeenCalled(); - }); - }); }); diff --git a/lib/assets/core/test/spec/cartodb3/deep-insights-integration/legend-manager.spec.js b/lib/assets/core/test/spec/cartodb3/deep-insights-integration/legend-manager.spec.js index e159e20194..fcac635135 100644 --- a/lib/assets/core/test/spec/cartodb3/deep-insights-integration/legend-manager.spec.js +++ b/lib/assets/core/test/spec/cartodb3/deep-insights-integration/legend-manager.spec.js @@ -100,7 +100,7 @@ describe('deep-insights-integrations/legend-manager', function () { cartocss: 'wadus' }); - expect(LegendFactory.createLegend).toHaveBeenCalledWith(this.layerDefinitionModel, 'bubble', {title: 'number'}); + expect(LegendFactory.createLegend).toHaveBeenCalledWith(this.layerDefinitionModel, 'bubble', {title: 'number', fillColor: '#045275'}); }); it('styles color', function () { @@ -124,7 +124,7 @@ describe('deep-insights-integrations/legend-manager', function () { }); expect(LegendFactory.createLegend).toHaveBeenCalledTimes(2); - expect(LegendFactory.createLegend).toHaveBeenCalledWith(this.layerDefinitionModel, 'bubble', {title: 'number'}); + expect(LegendFactory.createLegend).toHaveBeenCalledWith(this.layerDefinitionModel, 'bubble', {title: 'number', fillColor: '#ffc6c4'}); expect(LegendFactory.createLegend).toHaveBeenCalledWith(this.layerDefinitionModel, 'choropleth', {title: 'number'}); }); @@ -186,7 +186,7 @@ describe('deep-insights-integrations/legend-manager', function () { }); expect(LegendFactory.removeLegend).toHaveBeenCalledWith(this.layerDefinitionModel, 'custom_choropleth'); - expect(LegendFactory.createLegend).toHaveBeenCalledWith(this.layerDefinitionModel, 'bubble', {title: 'number'}); + expect(LegendFactory.createLegend).toHaveBeenCalledWith(this.layerDefinitionModel, 'bubble', {title: 'number', fillColor: '#045275'}); }); it('styles color', function () { @@ -223,7 +223,7 @@ describe('deep-insights-integrations/legend-manager', function () { expect(LegendFactory.removeLegend).toHaveBeenCalledWith(this.layerDefinitionModel, 'custom_choropleth', jasmine.any(Function)); expect(LegendFactory.createLegend).toHaveBeenCalledTimes(2); - expect(LegendFactory.createLegend).toHaveBeenCalledWith(this.layerDefinitionModel, 'bubble', {title: 'number'}); + expect(LegendFactory.createLegend).toHaveBeenCalledWith(this.layerDefinitionModel, 'bubble', {title: 'number', fillColor: '#ffc6c4'}); expect(LegendFactory.createLegend).toHaveBeenCalledWith(this.layerDefinitionModel, 'choropleth', {title: 'number'}); }); @@ -295,7 +295,7 @@ describe('deep-insights-integrations/legend-manager', function () { cartocss: 'wadus' }); - expect(LegendFactory.createLegend).toHaveBeenCalledWith(this.layerDefinitionModel, 'bubble', {title: 'number'}); + expect(LegendFactory.createLegend).toHaveBeenCalledWith(this.layerDefinitionModel, 'bubble', {title: 'number', fillColor: '#045275'}); }); it('styles color', function () { @@ -319,7 +319,7 @@ describe('deep-insights-integrations/legend-manager', function () { }); expect(LegendFactory.createLegend).toHaveBeenCalledTimes(2); - expect(LegendFactory.createLegend).toHaveBeenCalledWith(this.layerDefinitionModel, 'bubble', {title: 'number'}); + expect(LegendFactory.createLegend).toHaveBeenCalledWith(this.layerDefinitionModel, 'bubble', {title: 'number', fillColor: '#ffc6c4'}); expect(LegendFactory.createLegend).toHaveBeenCalledWith(this.layerDefinitionModel, 'choropleth', {title: 'number'}); }); @@ -379,7 +379,7 @@ describe('deep-insights-integrations/legend-manager', function () { cartocss: 'sudaw' }); - expect(LegendFactory.createLegend).toHaveBeenCalledWith(this.layerDefinitionModel, 'bubble', {title: 'number'}); + expect(LegendFactory.createLegend).toHaveBeenCalledWith(this.layerDefinitionModel, 'bubble', {title: 'number', fillColor: '#045275'}); }); it('styles color', function () { @@ -416,7 +416,7 @@ describe('deep-insights-integrations/legend-manager', function () { expect(LegendFactory.removeLegend).toHaveBeenCalledWith(this.layerDefinitionModel, 'custom_choropleth', jasmine.any(Function)); expect(LegendFactory.createLegend).toHaveBeenCalledTimes(2); - expect(LegendFactory.createLegend).toHaveBeenCalledWith(this.layerDefinitionModel, 'bubble', {title: 'number'}); + expect(LegendFactory.createLegend).toHaveBeenCalledWith(this.layerDefinitionModel, 'bubble', {title: 'number', fillColor: '#ffc6c4'}); expect(LegendFactory.createLegend).toHaveBeenCalledWith(this.layerDefinitionModel, 'choropleth', {title: 'number'}); }); @@ -650,7 +650,7 @@ describe('deep-insights-integrations/legend-manager', function () { // only once to create a bubble legend, but no choropleth legend expect(LegendFactory.createLegend).toHaveBeenCalledTimes(1); - expect(LegendFactory.createLegend).toHaveBeenCalledWith(this.layerDefinitionModel, 'bubble', {title: 'number'}); + expect(LegendFactory.createLegend).toHaveBeenCalledWith(this.layerDefinitionModel, 'bubble', {title: 'number', fillColor: '#ffc6c4'}); expect(LegendFactory.createLegend).not.toHaveBeenCalledWith(this.layerDefinitionModel, 'choropleth', {title: 'number'}); }); }); From f916c56f0d8cdbbafb5da7fb7469a9643c0e46a7 Mon Sep 17 00:00:00 2001 From: xavijam Date: Wed, 1 Mar 2017 10:56:00 +0100 Subject: [PATCH 4/4] News update --- NEWS.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/NEWS.md b/NEWS.md index 13e20a1675..1c32416bd8 100644 --- a/NEWS.md +++ b/NEWS.md @@ -56,7 +56,8 @@ Development * Migrate to use GNIP v2 for twitter search connector (#10051, #11595) ### Bug fixes -* Fixed missing metadata option in header when dataset is sync +* Fixed problem with Bubbles legend when a new analysis is applied (#11666) +* Fixed missing metadata option in header when dataset is sync (#11458) * Fixed problem with dates when filtering time series widget * Fixed problem switching between qualitative and quantitative attributes (#10654) * Fixed problem found in Surfaces related with map panning and widgets filtering