fix: multiple small bug fixes #1221

iBobik · 2022-09-17T23:50:38Z

I hope all fixes are ok :-)

Sorry for adding more commits in one PR. In my project I had to to many changes to make it work and I hope it is not big deal in project of this state.

I am open to specific feedback and will improve it ASAP, so they can be merged soon. You can also cherry pick some commits if it makes sense.

To be in sync with package.json. Previously: npm ci can only install packages when your package.json and package-lock.json or npm-shrinkwrap.json are in sync. Please update your lock file with npm install before continuing. Signed-off-by: Honza Pobořil <[email protected]>

Signed-off-by: Honza Pobořil <[email protected]>

Map load could take time and all child components will need to have map object loaded. Signed-off-by: Honza Pobořil <[email protected]>

Component could be mounted sooner than style finished loading, so check it first. Signed-off-by: Honza Pobořil <[email protected]>

Signed-off-by: Honza Pobořil <[email protected]>

- Added umnount hooks - Added visibility prop for geojson - Moved style load waiting from components to VMap Signed-off-by: Honza Pobořil <[email protected]>

So I can npm install branch directly. Signed-off-by: Honza Pobořil <[email protected]>

vinayakkulkarni · 2022-09-30T06:09:18Z

src/markers/VMarker.vue

-      map.value.on('style.load', () => {
-        // https://github.com/mapbox/mapbox-gl-js/issues/2268#issuecomment-401979967
-        const styleTimeout = () => {
-          if (!map.value.isStyleLoaded()) {
-            loaded.value = false;
-            setTimeout(styleTimeout, 200);
-          } else {
-            loaded.value = true;
-          }
-        };
-        styleTimeout();
-      });


If you've added the loaded check in map, maybe we can refactor this code from all the other files ?

based on the issue that is linked it seems that the style can be changed and then the map.value.isStyleLoaded() can return false even when the map was loaded once before. I think leaving this as is in VMarker and other components is safer then dooing it only in VMap component when load event is fired from mapbox.

The use case for this code is when you change the style of the map and then add new marker.

@Buk1m Do you think it would be ok to reload all child components (markers, …) centrally from map when style has changed?

Because this would allow simplify components.

@vinayakkulkarni Could you please tell if I can solve style change by children reload? It will be small performance degradation on style change, but huge simplification of child components.

Thank you for decision. I am going to work on another issues and I will need to merge this, because otherwise I will be forced to commit everything just to my fork and not care about PRs. It is hard to advocate open source contributions to my clients when it will delay fixes and make it more expensive.

Hi @iBobik, sorry for the late message I must have missed the previous notification.

I only worked with mapbox custom styles in a way that styles were loaded at the beginning and never changed, in which case your solution would work. (Un)fortunately you can change them after the map was loaded simply by calling map.setStyle and then map.value.isStyleLoaded() will return false again.

Loading new styles can take a while as it is usually a http request. So if the styles were changed and you will try to add new marker before loading is finished, then you will encounter "Style not done loading" error (At least according to the issue from comment).

There is no way to listen if styles were changed so you can either wrap setStyles method to know when the styles were changed and delay map manipulations until map.value.isStyleLoaded() return true or stick with the current solution.

So the case is:

Map has fully loaded once

Styles are changed

New marker is added before new styles are fully loaded.

Possible "Style not done loading" error

Keep in mind that the issue is old and I didn't test this exact case myself.

Do you have some idea how to solve it without having this timeout logic in every component?

Is it enough to have own method for style switching what will unload all children for the time of loading?

I am looking for solution what will simplify v-mapbox' codebase, because we are not able to do everything perfectly. In this project's status we have difficulties to push it to the stable state just for basic use-cases, so I think we should not paralyse development by requiring to cover too much use-cases.

iBobik · 2022-09-30T12:44:12Z

Yes, I haven't did it just because I does not use that components, so I can not test it.If there were working examples in docs it would be easier to test it all. What plans you have about it?Honza Pobořil💬Signal30. 9. 2022 v 8:09, Vinayak Kulkarni ***@***.***>: @vinayakkulkarni requested changes on this pull request. In src/markers/VMarker.vue:

- map.value.on('style.load', () => {

- // mapbox/mapbox-gl-js#2268 (comment) - const styleTimeout = () => { - if (!map.value.isStyleLoaded()) { - loaded.value = false; - setTimeout(styleTimeout, 200); - } else { - loaded.value = true; - } - }; - styleTimeout(); - }); If you've added the loaded check in map, maybe we can refactor this code from all the other files ? —Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you authored the thread.Message ID: ***@***.***>

Buk1m

The optional popup could be already merged but this PR contains too many changes to other files. The default popup is annoying as I cannot add a marker without popup or errors.

Most of the changes are nice in my opinion but the change to loaded event is unnecessary and can cause issues when map style is changed after initial load.

src/controls/VControlGeolocate.vue

src/layers/mapbox/VLayerMapboxGeojson.vue

Buk1m · 2022-10-12T09:01:21Z

src/markers/VMarker.vue

-      map.value.on('style.load', () => {
-        // https://github.com/mapbox/mapbox-gl-js/issues/2268#issuecomment-401979967
-        const styleTimeout = () => {
-          if (!map.value.isStyleLoaded()) {
-            loaded.value = false;
-            setTimeout(styleTimeout, 200);
-          } else {
-            loaded.value = true;
-          }
-        };
-        styleTimeout();
-      });


based on the issue that is linked it seems that the style can be changed and then the map.value.isStyleLoaded() can return false even when the map was loaded once before. I think leaving this as is in VMarker and other components is safer then dooing it only in VMap component when load event is fired from mapbox.

The use case for this code is when you change the style of the map and then add new marker.

Buk1m · 2022-10-12T09:14:23Z

I would like to use this library but it still needs more work and more reactivity. If I pass new coordinates to Marker then it should move on the map accordingly. I could make a PR with some improvements but first I would like to see this PR merged.

…ixes Signed-off-by: Honza Pobořil <[email protected]>

Signed-off-by: Honza Pobořil <[email protected]>

iBobik added 9 commits September 18, 2022 01:31

fix: component names in options to match filenames

71ee40c

Signed-off-by: Honza Pobořil <[email protected]>

fix: do not require options because default is enough

df7dbb0

Signed-off-by: Honza Pobořil <[email protected]>

fix: add empty render function to supress warning

1c14887

Signed-off-by: Honza Pobořil <[email protected]>

fix: resize map on load

d8ff847

Signed-off-by: Honza Pobořil <[email protected]>

fix: postpone child components load until map is loaded

27244d2

Map load could take time and all child components will need to have map object loaded. Signed-off-by: Honza Pobořil <[email protected]>

fix: check if style is loaded before adding layer

b78c396

Component could be mounted sooner than style finished loading, so check it first. Signed-off-by: Honza Pobořil <[email protected]>

fix: do not create popup if not used

9306b4e

Signed-off-by: Honza Pobořil <[email protected]>

fix: do not require prop because default is enough

fe2da64

Signed-off-by: Honza Pobořil <[email protected]>

iBobik changed the title ~~Multiple small bug fixes~~ fix: multiple small bug fixes Sep 18, 2022

iBobik added 2 commits September 19, 2022 02:01

fix: reactivity for Geojson and Marker

9b3839f

- Added umnount hooks - Added visibility prop for geojson - Moved style load waiting from components to VMap Signed-off-by: Honza Pobořil <[email protected]>

feat: add build to npm prepare

b58cfac

So I can npm install branch directly. Signed-off-by: Honza Pobořil <[email protected]>

vinayakkulkarni requested changes Sep 30, 2022

View reviewed changes

Buk1m reviewed Oct 12, 2022

View reviewed changes

iBobik added 2 commits January 30, 2023 01:17

Merge branch 'main' of https://github.com/geospoc/v-mapbox into bug-f…

c9dd2ef

…ixes Signed-off-by: Honza Pobořil <[email protected]>

style: removed redundant required and fixed whitespaces

496feef

Signed-off-by: Honza Pobořil <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: multiple small bug fixes #1221

fix: multiple small bug fixes #1221

iBobik commented Sep 17, 2022 •

edited

Loading

vinayakkulkarni Sep 30, 2022

Buk1m Oct 12, 2022

iBobik Nov 18, 2022

iBobik Jan 30, 2023

Buk1m Jan 30, 2023

iBobik Jan 30, 2023

iBobik commented Sep 30, 2022 via email

Buk1m left a comment

Buk1m Oct 12, 2022

Buk1m commented Oct 12, 2022

fix: multiple small bug fixes #1221

Are you sure you want to change the base?

fix: multiple small bug fixes #1221

Conversation

iBobik commented Sep 17, 2022 • edited Loading

vinayakkulkarni Sep 30, 2022

Choose a reason for hiding this comment

Buk1m Oct 12, 2022

Choose a reason for hiding this comment

iBobik Nov 18, 2022

Choose a reason for hiding this comment

iBobik Jan 30, 2023

Choose a reason for hiding this comment

Buk1m Jan 30, 2023

Choose a reason for hiding this comment

iBobik Jan 30, 2023

Choose a reason for hiding this comment

iBobik commented Sep 30, 2022 via email

Buk1m left a comment

Choose a reason for hiding this comment

Buk1m Oct 12, 2022

Choose a reason for hiding this comment

Buk1m commented Oct 12, 2022

iBobik commented Sep 17, 2022 •

edited

Loading