Skip to content

Allow removal and adding for plugins in iconMenu #861

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
oeninghe-dataport wants to merge 3 commits into
base: next
Choose a base branch
from
Open

Allow removal and adding for plugins in iconMenu #861

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e14ded9
feat(iconMenu): `menus` should be mutable
oeninghe-dataport Jul 6, 2026
8f36876
test(snowbox): allow removal and live insertion of iconMenu plugins
oeninghe-dataport Jul 6, 2026
4888d29
fix(filter): reset filter state on plugin removal
oeninghe-dataport Jul 6, 2026
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
1 change: 1 addition & 0 deletions examples/snowbox/index.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@ <h1>POLAR map client</h1>
</select>
</label>
<button id="color-scheme-switcher">Switch to dark mode</button>
<button id="toggle-filter">Add/Remove filter plugin</button>
<div>
Coordinates of currently selected feature: <span id="selected-feature-coordinates"></span>
</div>
Expand Down
13 changes: 13 additions & 0 deletions examples/snowbox/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,7 @@
colorScheme,
startCenter: [565874, 5934140],
layers: [
// TODO: Add internalization to snowbox

Check warning on line 95 in examples/snowbox/index.js

View workflow job for this annotation

GitHub Actions / Linting 

Unexpected 'todo' comment: 'TODO: Add internalization to snowbox'
{
id: basemapId,
visibility: true,
Expand Down Expand Up @@ -572,3 +572,16 @@
colorScheme = colorScheme === 'light' ? 'dark' : 'light'
updateState(map, 'core', 'colorScheme', colorScheme)
})

let filterItem = null
document
.getElementById('toggle-filter')
.addEventListener('click', ({ target }) => {
const store = getStore(map, 'iconMenu')
if (!filterItem) {
;[filterItem] = store.menus.splice(1, 1)
} else {
store.menus.splice(1, 0, filterItem)
filterItem = null
}
})
2 changes: 2 additions & 0 deletions src/plugins/filter/store.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,10 +52,12 @@ export const useFilterStore = defineStore('plugins/filter', () => {
{ deep: true, immediate: true }
)
)
teardownCallbacks.push(callback)
Comment thread
dopenguin marked this conversation as resolved.
})
}

function teardownPlugin() {
filterMainStore.state = {}
teardownCallbacks.forEach((callback) => {
callback()
})
Expand Down
9 changes: 1 addition & 8 deletions src/plugins/iconMenu/components/NineRegionsButton.ce.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@ import { storeToRefs } from 'pinia'
import { computed, inject } from 'vue'

import PolarIconButton from '@/components/PolarIconButton.ce.vue'
import { useCoreStore } from '@/core/stores'

import { useIconMenuStore } from '../store'

Expand All @@ -30,13 +29,7 @@ const active = computed(() => open.value === props.id)
const updateMaxWidth = inject('updateMaxWidth') as () => void

function toggle() {
if (open.value === props.id) {
open.value = null
useCoreStore().setMoveHandle(null)
} else {
open.value = props.id
iconMenuStore.openInMoveHandle(props.id)
}
open.value = open.value === props.id ? null : props.id
updateMaxWidth()
}
</script>
8 changes: 1 addition & 7 deletions src/plugins/iconMenu/components/StandardMenuList.ce.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,13 +122,7 @@ function updateMaxWidth() {
}

function toggle(id: string) {
if (open.value === id) {
open.value = null
coreStore.setMoveHandle(null)
} else {
open.value = id
iconMenuStore.openInMoveHandle(id)
}
open.value = open.value === id ? null : id
updateMaxWidth()
}
</script>
Expand Down
66 changes: 42 additions & 24 deletions src/plugins/iconMenu/store.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,14 +4,13 @@
*/
/* eslint-enable tsdoc/syntax */

import type { Component } from 'vue'
import type { Icon } from '@/core'
import type { Menu } from './types'

import { toMerged } from 'es-toolkit'
import { t } from 'i18next'
import { acceptHMRUpdate, defineStore } from 'pinia'
import { computed, markRaw, ref } from 'vue'
import { computed, markRaw, ref, watch } from 'vue'

import { useCoreStore } from '@/core/stores'

Expand Down Expand Up @@ -42,6 +41,33 @@ export const useIconMenuStore = defineStore('plugins/iconMenu', () => {
() => coreStore.configuration.iconMenu?.layoutTag ?? ''
)

const flatMenuItems = computed(() =>
menus.value.concat(focusMenus.value).flat()
)
watch(
flatMenuItems,
(newItems, oldItems) => {
const getDiffItems = (to, from) =>
to.filter(
(item) =>
!from.some((oldItem) => oldItem.plugin.id === item.plugin.id)
)
getDiffItems(newItems, oldItems).forEach((newItem) => {
coreStore.addPlugin(toMerged(newItem.plugin, { independent: false }))
})
getDiffItems(oldItems, newItems).forEach((oldItem) => {
if (open.value === oldItem.plugin.id) {
open.value = null
}
if (focusOpen.value === oldItem.plugin.id) {
focusOpen.value = null
}
coreStore.removePlugin(oldItem.plugin.id)
})
},
{ deep: true }
)
Comment on lines +47 to +69

@dopenguin dopenguin Jul 6, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I agree that iconMenu should handle the removal itself so the separation of concerns is not broken.

However, I would've thought adding its own removePlugin action that handles that procedure would be the way to go here. Note that, even though they are currently described otherwise in the export, neither menu nor focusMenus are considered stable API. I've made a note for myself to go over all plugins to adjust the exports to the requirements and mark most as @alpha.

@oeninghe-dataport oeninghe-dataport Jul 7, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

My first approach was to implement a addPlugin and removePlugin action. However, they get quite complicated, as you have to check which index and subindex to consider, remove an empty array if it is empty; and insertion similar.

If you prefer to have these actions, I can add them. I'd stick to this watcher to keep the more flexible option of direct editing, but we could e.g. mark only the actions as stable to keep menus as unstable API.

@dopenguin dopenguin Jul 7, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I prefer these actions, yeah.


function setupPlugin() {
menus.value = (coreStore.configuration.iconMenu?.menus || []).map(
(menuGroup) =>
Expand All @@ -64,24 +90,6 @@ export const useIconMenuStore = defineStore('plugins/iconMenu', () => {
coreStore.addPlugin(toMerged(plugin, { independent: false }))
})

// Otherwise, the component itself is made reactive
menus.value.map((menuGroup) =>
menuGroup.map((menuItem) =>
toMerged(menuItem, {
plugin: {
component: markRaw(menuItem.plugin.component as Component),
},
})
)
)
focusMenus.value.map((menuItem) =>
toMerged(menuItem, {
plugin: {
component: markRaw(menuItem.plugin.component as Component),
},
})
)

Comment on lines -67 to -84

@dopenguin dopenguin Jul 6, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The important part - making the component non-reactive with markRaw - seems to be lost on the new implementation.

@oeninghe-dataport oeninghe-dataport Jul 7, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

These lines have no effect anymore. Or do I miss anything?

const initiallyOpen = coreStore.configuration.iconMenu?.initiallyOpen
if (
!coreStore.hasSmallHeight &&
Expand All @@ -104,21 +112,31 @@ export const useIconMenuStore = defineStore('plugins/iconMenu', () => {

function openMenuById(openId: string) {
const entry = menus.value.flat().find(({ plugin: { id } }) => id === openId)

if (entry) {
open.value = openId
openInMoveHandle(openId)
}
}
watch(open, (open) => {
if (open) {
openInMoveHandle(open)
} else {
coreStore.setMoveHandle(null)
}
})
Comment on lines +119 to +125

@dopenguin dopenguin Jul 6, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I prefer the previously implemented version in favour of this side effect solution. Same goes for focusOpen.

What is the practical reason why this is needed or is simply personal preference?

@oeninghe-dataport oeninghe-dataport Jul 7, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Practical reason.

We had that logic already duplicated in NineRegionsButton and StandardMenuList, and I don't wanted to copy it a third time at https://github.com/Dataport/polar/pull/861/changes#diff-8793dc8dda9a07c08b84fcfa5edf13989ab72fde14867879719bfba5765c229aR59-R64

@dopenguin dopenguin Jul 7, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It could also be de-duplicated by abstracting the toggle functions of both components into an action.
I deem this favourable over the side-effect solution.


function openFocusMenuById(openId: string) {
const entry = focusMenus.value.find(({ plugin: { id } }) => id === openId)

if (entry) {
focusOpen.value = openId
openInMoveHandle(openId, true)
}
}
watch(focusOpen, (focusOpen) => {
if (focusOpen) {
openInMoveHandle(focusOpen, true)
} else {
coreStore.setMoveHandle(null)
}
})

function openInMoveHandle(openId: string, focusMenu = false) {
const menu = (focusMenu ? focusMenus.value : menus.value.flat()).find(
Expand Down
Loading