Fix accuracy circle turf geojson #5543
Conversation
this allows to handle much better all edge cases and eliminates calls to _updateCircleRadius at each user interaction with the map
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5543 +/- ##
===========================================
- Coverage 91.96% 69.35% -22.62%
===========================================
Files 282 286 +4
Lines 39077 39317 +240
Branches 6873 1359 -5514
===========================================
- Hits 35939 27268 -8671
- Misses 3010 11398 +8388
- Partials 128 651 +523 ☔ View full report in Codecov by Sentry. |
|
Is it reasonable to increase the bundle size with the small turfjs library? expected 912743 to be less than 910819 Later we may want to introduce parts of proj4js as well. I am wondering if it is a good way forward to accept small parts of other libraries in the build. |
CHANGELOG.md
Outdated
| @@ -15,6 +15,7 @@ | |||
| - Avoid setting marker opacity twice. ([#5441](https://github.com/maplibre/maplibre-gl-js/pull/5441)) | |||
|
|
|||
| ### 🐞 Bug fixes | |||
| - Fix location accuracy circle radius when moving view ([#5543](https://github.com/maplibre/maplibre-gl-js/pull/5543)) | |||
There was a problem hiding this comment.
This should not be here, but upwards instead...
package.json
Outdated
| @@ -55,6 +55,8 @@ | |||
| "@rollup/plugin-terser": "^0.4.4", | |||
| "@rollup/plugin-typescript": "^12.1.2", | |||
| "@stylistic/eslint-plugin-ts": "^4.0.1", | |||
| "@turf/circle": "^7.2.0", | |||
There was a problem hiding this comment.
Shouldn't this be under dependencies instead of here?
There was a problem hiding this comment.
I was thinking the same but it works and you cannot access turf outside of the build. But I will change it anyway. Maybe something to think about.
| @@ -610,6 +588,11 @@ describe('GeolocateControl with no options', () => { | |||
| map.addControl(geolocateControl); | |||
| // adding and removing to verify there is no race condition, and it is just added once | |||
| map.removeControl(geolocateControl); | |||
| await map.once('loaded', async () => { | |||
There was a problem hiding this comment.
use:
await map.once('loaded');
const accuracyElementsOnPage = ...
src/ui/control/geolocate_control.ts
Outdated
| @@ -265,7 +267,7 @@ export class GeolocateControl extends Evented implements IControl { | |||
| _watchState: 'OFF' | 'ACTIVE_LOCK' | 'WAITING_ACTIVE' | 'ACTIVE_ERROR' | 'BACKGROUND' | 'BACKGROUND_ERROR'; | |||
| _lastKnownPosition: any; | |||
| _userLocationDotMarker: Marker; | |||
| _accuracyCircleMarker: Marker; | |||
| _accuracyCirclePolygon: any; | |||
test/build/min.test.ts
Outdated
| @@ -38,7 +38,7 @@ describe('test min build', () => { | |||
| const decreaseQuota = 4096; | |||
|
|
|||
| // feel free to update this value after you've checked that it has changed on purpose :-) | |||
| const expectedBytes = 909795; | |||
| const expectedBytes = 912743; | |||
There was a problem hiding this comment.
I tend to think this is an overkill for such a small fix.
While the package size does grow over time, I'm not sure this component (which can be easily replaced by any app that requires better control over the UX) should cause this kind of change.
Can we write something simpler that doesn't require all the complexity of turf circle?
We did the same for simple ruler for example, where we took only the relevant functionality to reduce bundle increase...
There was a problem hiding this comment.
Isn' t this the way this fix was intended though. I am not sure if it should have turf in the title if it is not a fix using turf.
Should there maybe be an entirely new PR then?
Also, I don't know how to easily achieve a circle without turf. But I can look into it if that is what is best and desired now. Please let me know what you decide. I would recommend we accept the small increase but I wouldn't insist if bundle size is this important.
There was a problem hiding this comment.
Having thought about it again, I believe this just creates a Feature for you. I can think of how to do that directly with GeoJSON.
geekdenz
force-pushed
the
fix-accuracy-circle-turf-geojson
branch
from
4613fac to
d28b3f7
Compare
geekdenz
force-pushed
the
fix-accuracy-circle-turf-geojson
branch
from
43f32d6 to
c8c1d5e
Compare
geekdenz
force-pushed
the
fix-accuracy-circle-turf-geojson
branch
from
7cd4798 to
778a418
Compare
This PR takes up some of the work that was already done here:
#5440
The goal not finalised yet, is to get to a mergable PR.
Launch Checklist
CHANGELOG.mdunder the## mainsection.