-
Notifications
You must be signed in to change notification settings - Fork 0
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
CADENZA-36891: Extend API for geometry intersection #37
Conversation
@@ -244,7 +251,8 @@ | |||
layers: layers ? JSON.parse(layers) : undefined, | |||
useMapSrs: useMapSrs === 'on', | |||
fullGeometries: fullGeometries === 'on', | |||
zoomTarget: zoomToGeometry === 'on' ? { type: 'geometry' } : undefined | |||
zoomTarget: zoomToGeometry === 'on' ? { type: 'geometry' } : undefined, | |||
buffer: distance ? { value: Number(distance), lengthUnit: lengthUnit ? lengthUnit : 'm' } : undefined |
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.
- Why not just "unit"? Imo the "length" doesn't add anything here.
- The unit cannot be missing, because it's a that has a default value, right?
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.
lenghtUnit is just a little more precise for the current context, theoretically it could be also a degree unit
yes, 'm' (meters) is the default for the current context.
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 meant that
lengthUnit: lengthUnit ? lengthUnit : 'm'
can be simplified to
lengthUnit
because the lengthUnit
cannot be undefined.
{ useMapSrs, buffer, signal} = {}, | ||
) { | ||
this.#log('CadenzaClient#areaIntersection', ...arguments); | ||
const params = createParams({}); |
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.
Proposal: Rather make the "params" argument optional instead of passing a dummy value.
@type URLSearchParams | undefined
src/docs.md
Outdated
lengthUnit: 'm' | ||
} | ||
|
||
const response = await cadenzaClient.fetchAreaIntersection('embeddingTargetId', 'layerPrintName', geometry, { |
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.
Most of the examples in here use <placeholders>. I'd keep it consistent (and also fix the OI example).
src/docs.md
Outdated
bufferSize: bufferSize | ||
}); | ||
|
||
const featureCollection = await response; |
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.
Doesn't make sense to await the response twice (also in the OI example).
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.
Why not "just" assign the result to that variable straight-away?
const featureCollection = await ...
@@ -244,7 +251,8 @@ | |||
layers: layers ? JSON.parse(layers) : undefined, | |||
useMapSrs: useMapSrs === 'on', | |||
fullGeometries: fullGeometries === 'on', | |||
zoomTarget: zoomToGeometry === 'on' ? { type: 'geometry' } : undefined | |||
zoomTarget: zoomToGeometry === 'on' ? { type: 'geometry' } : undefined, | |||
buffer: distance ? { value: Number(distance), lengthUnit: lengthUnit ? lengthUnit : 'm' } : undefined |
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 meant that
lengthUnit: lengthUnit ? lengthUnit : 'm'
can be simplified to
lengthUnit
because the lengthUnit
cannot be undefined.
<textarea name="geometry" id="geometry" rows="5" required></textarea> | ||
<small> | ||
<p> | ||
The geometry must result in an area in combination with the buffer specification. |
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 far we try to assert things if possible before hitting the server.
src/docs.md
Outdated
bufferSize: bufferSize | ||
}); | ||
|
||
const featureCollection = await response; |
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.
Why not "just" assign the result to that variable straight-away?
const featureCollection = await ...
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.
approved
No description provided.