-
Notifications
You must be signed in to change notification settings - Fork 9
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
PB-729 : fix recent earthquake tooltip - #patch #988
Conversation
Passing run #2931 ↗︎Details:
Review all test suite changes for PR #988 ↗︎ |
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.
A few details I would change but that are not breaking anything. Good job 👍
@@ -49,6 +49,9 @@ onUnmounted(() => { | |||
}) | |||
|
|||
function getPixelForCoordinateFromMap() { | |||
if (!coordinates.value) { | |||
return |
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 kinda prefer when we are explicit with our returns (I know this works, and I know this returns undefined
... but here we know what is inside and we should return null
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 renamed the function to better explain what it does, having a get...
was suggesting it was supposed to return something, but that's not the case
const layer = store.getters.getLayerConfigById(parsedLayer.id) | ||
if (layer.isHighlightable) { | ||
// only highlightable feature will output something, for the others a click coordinate is required | ||
// (and we don't have it if we are here, as we are dealing with pre-selected feature in the URL at app startup) |
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.
So, I know layer
is already used outside of the scope but
I would move this out of the forEach, and have at line 124 somthing like :
124 : const layerConfig = store.getters.getLayerConfigById(parsedLayer.id)
125: if (features !== undefined && layerConfig.isHighlightable) { ...
It doesn't change that much, I must concede that, but I do find it a bit cleaner
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.
layer
was already computed above, so I removed the .getters.
call in the for loop and used the already present variable instead
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.
that's even better then
We were missing a `coord` param for the HTML popup endpoint (undocumented) so that the HTML endpoint can select features for us instead of giving feature IDs (not optimal, but that's the way it is for now) Also not requesting geometries (in identify requests) when the layer isn't highligtable, it was returning the extent of the whole layer as geom. Using instead the click coordinate as a geometry (adapting the code to be able to handle no geom)
fa96ee2
to
a2a3331
Compare
We were missing a
coord
param for the HTML popup endpoint (undocumented) so that the HTML endpoint can select features for us instead of giving feature IDs (not optimal, but that's the way it is for now)Also not requesting geometries (in identify requests) when the layer isn't highligtable, it was returning the extent of the whole layer as geom. Using instead the click coordinate as a geometry (adapting the code to be able to handle no geom)
Test link with recent earthquakes
Test link