More flow #5088
More flow #5088
Conversation
| @@ -35,7 +34,7 @@ module.exports = function (polygonRings, precision, debug) { | |||
| // a priority queue of cells in order of their "potential" (max distance to polygon) | |||
| const cellQueue = new Queue(null, compareMax); | |||
|
|
|||
| if (cellSize === 0) return [minX, minY]; | |||
| if (cellSize === 0) return new Point(minX, minY); | |||
There was a problem hiding this comment.
Flow found this bug.
src/ui/handler/drag_rotate.js
Outdated
| window.navigator.platform.toUpperCase().indexOf('MAC') >= 0) { | ||
| // Fix for https://github.com/mapbox/mapbox-gl-js/issues/3131: | ||
| // Firefox (detected by InstallTrigger) on Mac determines e.button = 2 when | ||
| // using Control + left click | ||
| eventButton = 0; | ||
| } | ||
| return (e.type === 'mousemove' ? e.buttons & buttons === 0 : !this.isActive() && eventButton !== button); | ||
| return (e.type === 'mousemove' ? e.buttons && buttons === 0 : !this.isActive() && eventButton !== button); |
There was a problem hiding this comment.
I changed this because flow was complaining, but I'm not sure it's correct... the intent is pretty obscure.
There was a problem hiding this comment.
Hmm, yeah. Peeking at the history, I actually suspect that this was intended to be (e.buttons & buttons) === 0, but I'm still not exactly sure what it's meant to be doing.
There was a problem hiding this comment.
I think (e.buttons & buttons) === 0 was indeed the intention, but in fact that prevents drag pan/rotate operations from working on Safari, which does not support the buttons property. Whereas e.buttons & buttons === 0 was simply always false, so mousemove events were never ignored, which is harmless!
To fix this, I'm going to remove the logic for ignoring mousemove events. Since it was never ignoring them, and we never noticed, it must be unnecessary.
src/ui/handler/drag_pan.js
Outdated
| @@ -197,7 +209,7 @@ class DragPanHandler { | |||
| if (e.ctrlKey) return true; | |||
| const buttons = 1, // left button | |||
| button = 0; // left button | |||
| return (e.type === 'mousemove' ? e.buttons & buttons === 0 : e.button && e.button !== button); | |||
| return (e.type === 'mousemove' ? e.buttons && buttons === 0 : e.button && e.button !== button); | |||
| @@ -386,7 +386,7 @@ class Camera extends Evented { | |||
| return 0; | |||
| }), ["bottom", "left", "right", "top"])) { | |||
| util.warnOnce("options.padding must be a positive number, or an Object with keys 'bottom', 'left', 'right', 'top'"); | |||
| return; | |||
| return this; | |||
There was a problem hiding this comment.
Flow found this bug.
| @@ -410,7 +410,7 @@ class Camera extends Evented { | |||
|
|
|||
| if (scaleY < 0 || scaleX < 0) { | |||
| util.warnOnce('Map cannot fit within canvas with the given bounds, padding, and/or offset.'); | |||
| return; | |||
| return this; | |||
There was a problem hiding this comment.
Flow found this bug.
There was a problem hiding this comment.
🎉 💯
One suggestion -- alternative to (null: any) by typing a couple properties as maybes: 0229a56
| @@ -967,7 +1076,7 @@ function getSizeVertexData(layer, tileZoom, stopZoomLevels, sizeProperty, featur | |||
| ) { | |||
| // source function | |||
| return [ | |||
| 10 * layer.getLayoutValue(sizeProperty, {}, featureProperties) | |||
| 10 * layer.getLayoutValue(sizeProperty, ({} : any), featureProperties) | |||
There was a problem hiding this comment.
This could be null instead of ({}: any), but we can get that later.
src/ui/handler/drag_rotate.js
Outdated
| window.navigator.platform.toUpperCase().indexOf('MAC') >= 0) { | ||
| // Fix for https://github.com/mapbox/mapbox-gl-js/issues/3131: | ||
| // Firefox (detected by InstallTrigger) on Mac determines e.button = 2 when | ||
| // using Control + left click | ||
| eventButton = 0; | ||
| } | ||
| return (e.type === 'mousemove' ? e.buttons & buttons === 0 : !this.isActive() && eventButton !== button); | ||
| return (e.type === 'mousemove' ? e.buttons && buttons === 0 : !this.isActive() && eventButton !== button); |
There was a problem hiding this comment.
Hmm, yeah. Peeking at the history, I actually suspect that this was intended to be (e.buttons & buttons) === 0, but I'm still not exactly sure what it's meant to be doing.
| @@ -1156,7 +1184,7 @@ class Map extends Camera { | |||
| image: HTMLImageElement | $ArrayBufferView, | |||
| options?: {width: number, height: number, pixelRatio: number} | |||
| ) { | |||
| this.style.spriteAtlas.addImage(name, image, options); | |||
| this.style.spriteAtlas.addImage(name, image, (options : any)); | |||
There was a problem hiding this comment.
Should we just type options as optional on SpriteAtlas.addImage as well?
There was a problem hiding this comment.
I'm rewriting this code as part of #4876; will be addressed there.
|
@jfirebaugh whoops the test failure is from a mistake in my suggested change. Fix here: 3567a61 |
|
Going to go ahead and revert that; we should do a general pass over |
Up to 89% coverage now!