-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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.
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
Bug fix: implement onLoad event calling when reuseMaps is set to true #704
Bug fix: implement onLoad event calling when reuseMaps is set to true #704
Conversation
src/mapbox/mapbox.js
Outdated
// TODO - need to call onload again, need to track with Promise? | ||
props.onLoad(); | ||
// call onload event handler after style fully loaded when style needs update | ||
this._map.once('style.load', props.onLoad); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we know for sure that this event will fire even if props.mapStyle
is the same as the last style set?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. We didn't pass the second parameter into .setStyle
method (default to false
), so we would let mapbox force update even if the style is the same as before, so the event will be fired al the time (in this situation).
Descriptions for options
inside .setStyle(style, options)
options.diff: If false, force a 'full' update, removing the current style and building the given one instead of attempting a diff-based update. from https://docs.mapbox.com/mapbox-gl-js/api/#map#setstyle
However, I think there's some other things need to do here:
- We don't need to
setStyle
if current map style is already the things we want (by set options.diff totrue
); - Because we add step one, so we need to check if the style is fully loaded when we want to manually fire
onLoad
event (by addisStyleLoaded
judgement); - I got a warning of
Method '_create' has too many statements (26). Maximum allowed is 25.
after I coded the logic above, so I change some code inside the judgement mentioned in step two to a ternary expression;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One more step didn't mentioned above:
We should listen styledata
event rather than style.load
event, since later one is a private event inside mapbox, as discussed in mapbox/mapbox-gl-js#7579 (comment) .
Actually, there's 5 more things in this new commit, the fifth one is here: I also unified inputs of That's because developers define It may not be a perfect solution but I think it's the best one right now, a better solution is to have a more precise event types (pretty the same as #683 (comment) ), then we can handler A pseudocode should like this inside
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. Thank you!
src/mapbox/mapbox.js
Outdated
}); | ||
|
||
// call onload event handler after style fully loaded when style needs update | ||
(this._map.isStyleLoaded() ? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This syntax is rather odd. Just use if
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I got a warning of Method '_create' has too many statements (26). Maximum allowed is 25. if I use if statement, that’s why I choose this one syntax.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can add this line to the top of this method to disable the lint rule:
/* eslint-disable max-statements */
I will refactor it after the patch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that's correct since it will overwrite the global settings. I can handle it by adding another commit.
BTW you can use examples/reuse-map to test this feature. |
4 more implements with:
|
examples/main/constants/toc.js
Outdated
path: 'heatmap', | ||
name: 'Heatmap', | ||
component: Heatmap | ||
>>>>>>> upstream/master |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for this, I'll resolve it by adding another commit.
Reusemap example is not on the website because it does not meet the visual quality bar. Can you remove it for now so that we can release? |
Removed and by the way, is there any specific guidelines of the visual quality bar (I'm interested)? |
Since this is a wrapper library, we try to put up examples that replicate existing mapbox-gl-js examples or at least along the same lines. The reuse-map example in particular was made for performance profiling/debugging purposes so we never bothered to make it look good. |
Description and resolution
According to the discussion in #699 , there's a TODO inside mapbox.js, which is calling onLoad event again when map instance is re-created again with its' property
reuseMaps
set to true.However, we need to handle two situations here:
setStyle
with customizedmapStyle
: In this situation, we need to wait until style is loaded, and then callonLoad
event handler; we choose.once
API here to listenstyle.load
event;setStyle
with customizedmapStyle
: In this situation, we can directly generate anevent
object and pass it toprops.onLoad
method and call it right now;Tests and documentation
npm run test
passed with 222 tests.