Skip to content

fix: multiple small bug fixes #1221

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.

Sign up for GitHub

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

Open
wants to merge 13 commits into
base: main
Choose a base branch
from
Open

fix: multiple small bug fixes #1221

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
a68738d
fix: updated lock file
iBobik Sep 17, 2022
71ee40c
fix: component names in options to match filenames
iBobik Sep 17, 2022
df7dbb0
fix: do not require options because default is enough
iBobik Sep 17, 2022
1c14887
fix: add empty render function to supress warning
iBobik Sep 17, 2022
d8ff847
fix: resize map on load
iBobik Sep 17, 2022
27244d2
fix: postpone child components load until map is loaded
iBobik Sep 17, 2022
b78c396
fix: check if style is loaded before adding layer
iBobik Sep 17, 2022
9306b4e
fix: do not create popup if not used
iBobik Sep 17, 2022
fe2da64
fix: do not require prop because default is enough
iBobik Sep 17, 2022
9b3839f
fix: reactivity for Geojson and Marker
iBobik Sep 19, 2022
b58cfac
feat: add build to npm prepare
iBobik Sep 19, 2022
c9dd2ef
Merge branch 'main' of https://github.com/geospoc/v-mapbox into bug-f…
iBobik Jan 30, 2023
496feef
style: removed redundant `required` and fixed whitespaces
iBobik Jan 30, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8,739 changes: 3,514 additions & 5,225 deletions package-lock.json
View file
Open in desktop

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@
"bundle": "rollup -c build/rollup.config.ts",
"bundle:min": "rollup -c build/rollup.config.min.ts",
"build:dts": "vue-tsc --declaration --emitDeclarationOnly --skipLibCheck && npm run lintfix",
"prepare": "husky install",
"prepare": "husky install && npm run build",
"docs:dev": "vuepress dev docs --port 3000",
"docs:build": "vuepress build docs",
"lint": "npm run lint:prettier && npm run lint:eslint && npm run lint:css",
Expand Down
1 change: 0 additions & 1 deletion src/controls/VControlAttribution.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,6 @@
'top-right' | 'top-left' | 'bottom-right' | 'bottom-left'
>,
default: () => 'bottom-right',
required: false,
},
},
setup(props, { slots }) {
Expand Down
1 change: 0 additions & 1 deletion src/controls/VControlFullscreen.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@
'top-right' | 'top-left' | 'bottom-right' | 'bottom-left'
>,
default: () => 'top-left',
required: false,
},
},
setup(props) {
Expand Down
5 changes: 2 additions & 3 deletions src/controls/VControlGeolocate.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
import { injectStrict, MapKey } from '../utils';

export default defineComponent({
name: 'VControlFullscreen',
name: 'VControlGeolocate',
props: {
options: {
type: Object as PropType<{
Expand All @@ -18,14 +18,12 @@
showUserLocation?: boolean;
}>,
default: () => ({}),
required: true,
},
position: {
type: String as PropType<
'top-right' | 'top-left' | 'bottom-right' | 'bottom-left'
>,
default: () => 'top-left',
required: false,
},
},
setup(props, { emit }) {
Expand All @@ -50,5 +48,6 @@
});
}
},
render: () => [],
});
</script>
3 changes: 1 addition & 2 deletions src/controls/VControlNavigation.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
import { injectStrict, MapKey } from '../utils';

export default defineComponent({
name: 'VControlFullscreen',
name: 'VControlNavigation',
props: {
options: {
type: Object as PropType<{
Expand All @@ -21,7 +21,6 @@
'top-right' | 'top-left' | 'bottom-right' | 'bottom-left'
>,
default: () => 'top-left',
required: false,
},
},
setup(props) {
Expand Down
3 changes: 1 addition & 2 deletions src/controls/VControlScale.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
import { injectStrict, MapKey } from '../utils';

export default defineComponent({
name: 'VControlFullscreen',
name: 'VControlScale',
props: {
options: {
type: Object as PropType<{ maxWidth?: number; unit?: string }>,
Expand All @@ -17,7 +17,6 @@
'top-right' | 'top-left' | 'bottom-right' | 'bottom-left'
>,
default: () => 'bottom-left',
required: false,
},
},
setup(props) {
Expand Down
1 change: 0 additions & 1 deletion src/layers/deck.gl/VLayerDeckArc.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,6 @@
before: {
type: String as PropType<string>,
default: '',
required: false,
},
},
setup(props) {
Expand Down
1 change: 0 additions & 1 deletion src/layers/deck.gl/VLayerDeckGeojson.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,6 @@
before: {
type: String as PropType<string>,
default: '',
required: false,
},
},
setup(props) {
Expand Down
1 change: 0 additions & 1 deletion src/layers/mapbox/VLayerMapboxCanvas.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,6 @@
before: {
type: String as PropType<string>,
default: '',
required: false,
},
},
setup(props) {
Expand Down
59 changes: 24 additions & 35 deletions src/layers/mapbox/VLayerMapboxGeojson.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,8 @@
</template>
<script lang="ts">
import type { AnyLayer, GeoJSONSourceRaw } from 'mapbox-gl';
import type { PropType, Ref } from 'vue';
import { defineComponent, onMounted, ref, watch } from 'vue';
import type { PropType } from 'vue';
import { defineComponent, onMounted, onUnmounted, watch } from 'vue';
import { injectStrict, MapKey } from '../../utils';

export default defineComponent({
Expand Down Expand Up @@ -34,54 +34,43 @@
before: {
type: String as PropType<string>,
default: '',
required: false,
},
visible: {
type: Boolean as PropType<boolean>,
default: true,
},
},
setup(props) {
let map = injectStrict(MapKey);
let loaded: Ref<boolean> = ref(false);

const layer = {
...props.layer,
id: props.layerId,
source: props.sourceId,
};

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();
});

/**
* Watcher(s)
*/
watch(loaded, (value) => {
if (value) {
addLayer();
}
onMounted(() => {
map.value.addSource(props.sourceId, props.source);
map.value.addLayer(layer, props.before);
});

onMounted(() => {
addLayer();
onUnmounted(() => {
if (map.value.getLayer(props.layerId))
map.value.removeLayer(props.layerId);
if (map.value.getSource(props.sourceId))
map.value.removeSource(props.sourceId);
});

/**
* Re–adds the layer when style changed
*
* @returns {void}
*/
function addLayer(): void {
map.value.addSource(props.sourceId, props.source);
map.value.addLayer(layer, props.before);
}
watch(
() => props.visible,
(visible) => {
map.value.setLayoutProperty(
props.layerId,
'visibility',
visible ? 'visible' : 'none',
);
},
);
},
});
</script>
1 change: 0 additions & 1 deletion src/layers/mapbox/VLayerMapboxImage.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,6 @@
before: {
type: String as PropType<string>,
default: '',
required: false,
},
},
setup(props) {
Expand Down
1 change: 0 additions & 1 deletion src/layers/mapbox/VLayerMapboxRaster.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,6 @@
before: {
type: String as PropType<string>,
default: '',
required: false,
},
},
setup(props) {
Expand Down
1 change: 0 additions & 1 deletion src/layers/mapbox/VLayerMapboxVector.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,6 @@
before: {
type: String as PropType<string>,
default: '',
required: false,
},
},
setup(props) {
Expand Down
1 change: 0 additions & 1 deletion src/layers/mapbox/VLayerMapboxVideo.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,6 @@
before: {
type: String as PropType<string>,
default: '',
required: false,
},
},
setup(props) {
Expand Down
18 changes: 14 additions & 4 deletions src/map/VMap.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
<template>
<div :id="getContainer()" class="v-map-container">
<slot />
<slot v-if="loaded" />
</div>
</template>
<script lang="ts">
Expand All @@ -22,7 +22,7 @@
},
setup(props, { emit }: SetupContext) {
let map: Ref<Map> = ref({} as Map);
let loaded: Ref<boolean> = ref(false);
let loaded = ref(false);
let events: Ref<Array<keyof MapEventType>> = ref(mapEvents);

onMounted(() => {
Expand All @@ -31,7 +31,6 @@
? props.options
: { ...props.options, container: 'map' };
map.value = new Map(options);
loaded.value = true;
provide(MapKey, map);
listenMapEvents();
});
Expand All @@ -47,7 +46,17 @@
map.value.on(e, (evt) => {
switch (e) {
case 'load':
emit('loaded', map.value);
// https://github.com/mapbox/mapbox-gl-js/issues/2268#issuecomment-401979967
const styleTimeout = () => {
if (!map.value.isStyleLoaded()) {
setTimeout(styleTimeout, 200);
} else {
loaded.value = true;
map.value.resize();
emit('loaded', map.value);
}
};
styleTimeout();
break;
default:
emit(e, evt);
Expand All @@ -71,6 +80,7 @@

return {
getContainer,
loaded,
};
},
});
Expand Down
42 changes: 14 additions & 28 deletions src/markers/VMarker.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
<template>
<section :id="`marker-${Date.now()}`">
<section v-if="hasPopup" :id="`marker-${Date.now()}`">
<v-popup
:marker="marker"
:options="popupOptions"
Expand All @@ -17,8 +17,8 @@
PopupOptions,
} from 'mapbox-gl';
import { Marker } from 'mapbox-gl';
import type { PropType, Ref, SetupContext } from 'vue';
import { defineComponent, onMounted, ref } from 'vue';
import type { PropType, SetupContext } from 'vue';
import { computed, defineComponent, onMounted, onUnmounted } from 'vue';
import { markerDOMEvents, markerMapEvents } from '../constants/events';
import VPopup from '../popups/VPopup.vue';
import { injectStrict, MapKey } from '../utils';
Expand All @@ -32,12 +32,10 @@
options: {
type: Object as PropType<MarkerOptions>,
default: () => ({} as MarkerOptions),
required: true,
},
popupOptions: {
type: Object as PropType<PopupOptions>,
default: () => ({} as PopupOptions),
required: true,
},
coordinates: {
type: [Object, Array] as PropType<LngLatLike>,
Expand All @@ -47,38 +45,25 @@
cursor: {
type: String as PropType<string>,
default: 'pointer',
required: false,
},
},
setup(props, { emit }: SetupContext) {
setup(props, { emit, slots }: SetupContext) {
let map = injectStrict(MapKey);
let marker: Marker = new Marker(props.options);
let loaded: Ref<boolean> = ref(true);

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();
});
Comment on lines -58 to -69
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. Map has fully loaded once
  2. Styles are changed
  3. New marker is added before new styles are fully loaded.
  4. Possible "Style not done loading" error

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


onMounted(() => {
if (loaded.value) {
setMarkerCoordinates();
addToMap();
setCursorPointer();
} else {
removeFromMap();
}
setMarkerCoordinates();
addToMap();
setCursorPointer();
listenMarkerEvents();
});

onUnmounted(() => {
removeFromMap();
});

const hasPopup = computed(() => !!slots.default?.length);

/**
* Set marker coordinates
*
Expand Down Expand Up @@ -145,6 +130,7 @@
}

return {
hasPopup,
marker,
};
},
Expand Down
1 change: 0 additions & 1 deletion src/popups/VPopup.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@
marker: {
type: Object as PropType<Marker>,
default: () => ({} as Marker),
required: false,
},
options: {
type: Object as PropType<PopupOptions>,
Expand Down
2 changes: 0 additions & 2 deletions types/controls/VControlAttribution.vue.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@ declare const _default: import('vue').DefineComponent<
position: {
type: PropType<'top-right' | 'top-left' | 'bottom-right' | 'bottom-left'>;
default: () => string;
required: false;
};
},
void,
Expand Down Expand Up @@ -47,7 +46,6 @@ declare const _default: import('vue').DefineComponent<
'top-right' | 'top-left' | 'bottom-right' | 'bottom-left'
>;
default: () => string;
required: false;
};
}>
>,
Expand Down
Loading