Prevent fetching grid.json tiles in embeds with no interactive layers. #1979
Conversation
| return false; | ||
| } | ||
| // Builder needs interactivity when the layer has no popup/infowindows to allow feature edition. | ||
| return layerModel.isInteractive() || this._mapModel.isFeatureInteractivityEnabled(); |
There was a problem hiding this comment.
The second condition has little to do with the layer as the name of this method suggests so I would extract it from this method. It might also make sense to include the isVisible part inside of isInteractive? Perhaps not...
There was a problem hiding this comment.
yep, the name is not the best... maybe _shouldEnableInteraction(layer) ?
- Check if a layer is interactive or the map has the interactivity forced - Update cartodb-layer-group tests - Add leaflet tests
IagoLast
force-pushed
the
1976-prevent-gridjson
branch
from
885fd1d
to
d537d09
Compare
| @@ -57,6 +66,7 @@ CartoDBLayerGroupViewBase.prototype = { | |||
| .on('off', function (o) { | |||
| if (self._interactionDisabled) return; | |||
| o = o || {}; | |||
| // TODO: zera has an .on('error', () => { }) callback that should be used here | |||
There was a problem hiding this comment.
... that should be used here -> Why don't we use it then?
There was a problem hiding this comment.
Since the current issue is generating multiple errors in the track.js and we want to fix it asap IMO we should address the zera thing in a different issue.
| }); | ||
| }); | ||
|
|
||
| xdescribe("when there aren't tile URLS", function () { |
| }); | ||
|
|
||
| it('should trigger a "featureError" event when interactivity triggers a featureOut event', function (done) { | ||
| pending('Not implemented yet'); |
|
|
||
| xdescribe("when there aren't tile URLS", function () { | ||
| it('should fetch empty tiles', function () { | ||
| pending('how to compare the empty gif against the tile url?'); |
|
|
||
| it('should enable interaction when there are interactive layers ', function () { | ||
| mapModelMock.isFeatureInteractivityEnabled.and.returnValue(false); | ||
| spyOn(cartoDbLayer0, 'isInteractive').and.returnValue(true); |
There was a problem hiding this comment.
Perhaps we can move this spy to the beforeEach so that we don't have to repeat it in other it blocks.
| @@ -1,389 +0,0 @@ | |||
| var _ = require('underscore'); | |||
There was a problem hiding this comment.
Why are we removing this and duplicating the tests instead?
There was a problem hiding this comment.
This was/is kind of convenient IMO.
There was a problem hiding this comment.
Jasmine logs/workflow are easier to read this way (imo)
There was a problem hiding this comment.
I left some comments about error handling and tests, but since this is important, we can tackle those in a follow up PR. All yours @IagoLast.
#1976