Feature/draw ol#284
Conversation
|
|
||
| /** Get a GeoJSON feature by ID, or null. */ | ||
| get (id) { | ||
| const feature = source.getFeatureById(String(id)) |
There was a problem hiding this comment.
why not this.getOL(id) - to avoid repeating this line throughout
There was a problem hiding this comment.
I'll do that, thanks
| /** Add or replace a GeoJSON feature. Returns the OL Feature. */ | ||
| add (geojsonFeature) { | ||
| const existing = source.getFeatureById(geojsonFeature.id) | ||
| if (existing) source.removeFeature(existing) |
There was a problem hiding this comment.
how about:
const existing = this.getOL(geojsonFeature.id)
avoids repeating the line
| const ctx = state.context | ||
| const pr = state.pixelRatio | ||
| const [cx, cy] = /** @type {number[]} */ (pixelCoordinates) | ||
| ctx.save() |
There was a problem hiding this comment.
// could you replace the following code with a function that draws the same thing but is reusable?
ctx.beginPath(); ctx.arc(cx, cy, outer * pr, 0, Math.PI * 2); ctx.fillStyle = colors.editActive; ctx.fill()
ctx.beginPath(); ctx.arc(cx, cy, mid * pr, 0, Math.PI * 2); ctx.fillStyle = colors.editHalo; ctx.fill()
ctx.beginPath(); ctx.arc(cx, cy, inner * pr, 0, Math.PI * 2); ctx.fillStyle = colors[innerKey]; ctx.fill()
drawContextPath(ctx, cx, cy, whateverThisShouldBeCalled, fillStyle) {
ctx.beginPath()
ctx.arc(cx, cy, whateverThisShouldBeCalled * pr, 0, Math.PI * 2)
ctx.fillStyle = fillStyle
ctx.fill()
}
// So you would have:
drawContextPath(ctx, cx, cy, outer, colors.editActive)
drawContextPath(ctx, cx, cy, mid, colors.editHalo)
drawContextPath(ctx, cx, cy, inner, colors[innerKey])
There was a problem hiding this comment.
Absolutely, much better
| } | ||
|
|
||
| const nudge = (e) => { | ||
| const { selectedVertexIndex, selectedVertexType, vertecies, midpoints, olFeature } = getState() |
There was a problem hiding this comment.
vertecies spelling should be vertices
| const offsets = { ArrowUp: [0, -step], ArrowDown: [0, step], ArrowLeft: [-step, 0], ArrowRight: [step, 0] } | ||
| const [dx, dy] = offsets[e.key] | ||
|
|
||
| if (selectedVertexType === 'midpoint') { |
There was a problem hiding this comment.
would a nudgeMidpoint method here be better, than a large if and early return
| // Escape if snap is preventing sufficient progress in the intended direction. | ||
| // Covers vertex-stuck (newCoord === current) and edge-hugging (vertex slides | ||
| // along edge instead of moving away from it). | ||
| if (snap) { |
There was a problem hiding this comment.
I can't see where snap is defined - should it be this.snap?
could this be a method snapCoord(...)
There was a problem hiding this comment.
It comes in through the function args
| const numPairs = (end - start) / 2 | ||
| const edgeCount = isClosedRing ? numPairs : numPairs - 1 | ||
|
|
||
| for (let i = 0; i < numPairs; i++) { |
There was a problem hiding this comment.
move to a getBestPair method?
| } | ||
| } | ||
|
|
||
| for (let i = 0; i < edgeCount; i++) { |
There was a problem hiding this comment.
move to a getBestEdge method
markfee
left a comment
markfee
left a comment
There was a problem hiding this comment.
few suggestions.
The HUGE prs are creeping in again, please try to keep them small.
a few suggestions inline,
Things like keyboardActions / touch actions might be better decoupled from the effect.
Ie - move the effect, such as nudgingPoints out, and just have the keyboardActions call the effect.
|
2 participants
No description provided.