added zoom level to reloading images #964
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
My layers were reappearing when we set definition queries on invisible layers
nickpeihl
reviewed
Jun 8, 2017
| @@ -240,7 +240,9 @@ export var FeatureManager = VirtualGrid.extend({ | |||
| // schedule adding features for the next animation frame | |||
| Util.requestAnimFrame(Util.bind(function () { | |||
| this.removeLayers(oldSnapshot); | |||
| this.addLayers(newSnapshot); | |||
| if (zoom < this.options.maxZoom && zoom > this.options.minZoom) { | |||
There was a problem hiding this comment.
The zoom variable here is undefined. Is this supposed to be the current zoom level of the map (i.e. map.getZoom())?
|
My fault... copy and paste error.... I think i fixed it
…On Thu, Jun 8, 2017 at 2:56 PM, Nick Peihl ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In src/Layers/FeatureLayer/FeatureManager.js
<#964 (comment)>:
> @@ -240,7 +240,9 @@ export var FeatureManager = VirtualGrid.extend({
// schedule adding features for the next animation frame
Util.requestAnimFrame(Util.bind(function () {
this.removeLayers(oldSnapshot);
- this.addLayers(newSnapshot);
+ if (zoom < this.options.maxZoom && zoom > this.options.minZoom) {
The zoom variable here is undefined. Is this supposed to be the current
zoom level of the map (i.e. map.getZoom()
<http://leafletjs.com/reference-1.0.3.html#map-getzoom>)?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#964 (review)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABLNpr2krpIhH-roBOTbqDEuvc2DdWG9ks5sCGANgaJpZM4N0E_O>
.
--
|
|
thx @jordanparfitt! i'm traveling at the moment, but i'll be able to review this soon. |
jgravois
suggested changes
Jun 15, 2017
There was a problem hiding this comment.
thanks for catching this.
the fix would be more DRY if you reused FeatureManager._visibleZoom()
it'd also be a bit tidier to append the condition to the exterior conditional block thats already present.
if (pendingRequests <= 0 && _visibleZoom()) {used built in method _visibleZoom() instead of cheacking map
jgravois
suggested changes
Jun 15, 2017
| @@ -235,12 +235,12 @@ export var FeatureManager = VirtualGrid.extend({ | |||
|
|
|||
| pendingRequests--; | |||
|
|
|||
| if (pendingRequests <= 0) { | |||
| if (pendingRequests <= 0 && _visibleZoom()) { | |||
There was a problem hiding this comment.
as you can see in the failing test, _visibleZoom isn't defined.
that was a typo on my part. you need to use this._visibleZoom.
| this._currentSnapshot = newSnapshot; | ||
| // schedule adding features for the next animation frame | ||
| Util.requestAnimFrame(Util.bind(function () { | ||
| this.removeLayers(oldSnapshot); | ||
| this.addLayers(newSnapshot); | ||
| this.addLayers(newSnapshot); |
There was a problem hiding this comment.
just a tad too much indentation here.
Fixed indentation, added this. to be consistent with other calls to _visibleZoom()
|
Why'd it fail?
…On Thu, Jun 15, 2017 at 3:17 PM, john gravois ***@***.***> wrote:
***@***.**** requested changes on this pull request.
let me know after you're able to test my own changes locally and confirm
that they have the appropriate affect (and don't cause any other problems).
------------------------------
In src/Layers/FeatureLayer/FeatureManager.js
<#964 (comment)>:
> @@ -235,12 +235,12 @@ export var FeatureManager = VirtualGrid.extend({
pendingRequests--;
- if (pendingRequests <= 0) {
+ if (pendingRequests <= 0 && _visibleZoom()) {
as you can see in the failing test
<https://travis-ci.org/Esri/esri-leaflet/builds/243430000>, _visibleZoom
isn't defined.
that was a typo on my part. you need to use this._visibleZoom.
------------------------------
In src/Layers/FeatureLayer/FeatureManager.js
<#964 (comment)>:
> this._currentSnapshot = newSnapshot;
// schedule adding features for the next animation frame
Util.requestAnimFrame(Util.bind(function () {
this.removeLayers(oldSnapshot);
- this.addLayers(newSnapshot);
+ this.addLayers(newSnapshot);
just a tad too much indentation here.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#964 (review)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABLNpusIRYGC6D5jVRFLiE50FYwoADasks5sEZ9hgaJpZM4N0E_O>
.
|
jgravois
approved these changes
Jun 15, 2017
|
|
I double checked this morning and it fixed my issue with the new changes. Thanks! |
jgravois
added a commit
to jgravois/esri-leaflet
that referenced
this pull request
Apr 23, 2022
added conditional check to avoid reloading features outside a constrained zoom level when new SQL filters are applied.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
My layers were reappearing when we set definition queries on invisible layers