Skip to content

Allow removal and adding for plugins in iconMenu - #861

Open
oeninghe-dataport wants to merge 9 commits into
nextfrom
fix/668-iconMenu-plugin-removal
Open

Allow removal and adding for plugins in iconMenu#861
oeninghe-dataport wants to merge 9 commits into
nextfrom
fix/668-iconMenu-plugin-removal

Conversation

@oeninghe-dataport

Copy link
Copy Markdown
Collaborator

Summary

Plugins in iconMenu can be added and removed at runtime.

Note:
coreStore.removePlugin(...) is still not supported for plugins within iconMenu. I deem this correct. As iconMenu adds the plugin, it should also be responsible for removal of the plugin. Otherwise, we'd need to breach the separation of core and plugins.

Instructions for local reproduction and review

  • Open snowbox.
  • Add the layer "Anliegen (MML)"
  • Open filter plugin.
  • Filter out some "Anliegen".
  • Press the newly-introduced "Add/Remove filter plugin" button.

Relevant tickets, issues, et cetera

Fixes #668

@oeninghe-dataport oeninghe-dataport added this to the POLAR@3 milestone Jul 6, 2026
@oeninghe-dataport oeninghe-dataport self-assigned this Jul 6, 2026
@oeninghe-dataport
oeninghe-dataport requested a review from a team July 6, 2026 08:38
@oeninghe-dataport oeninghe-dataport added the bug Something isn't working label Jul 6, 2026
@github-actions

github-actions Bot commented Jul 6, 2026

Copy link
Copy Markdown
PR Preview Action v1.8.1

QR code for preview link

🚀 View preview at
https://Dataport.github.io/polar/pr-preview/pr-861/

Built to branch gh-pages at 2026-07-29 18:05 UTC.
Preview will be ready when the GitHub Pages deployment is complete.

@dopenguin dopenguin left a comment

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.

Comment thread src/plugins/filter/store.ts
Comment thread src/plugins/iconMenu/store.ts Outdated
Comment thread src/plugins/iconMenu/store.ts Outdated
Comment thread src/plugins/iconMenu/store.ts Outdated
@oeninghe-dataport

Copy link
Copy Markdown
Collaborator Author

🏓 @dopenguin

@dopenguin

Copy link
Copy Markdown
Member

🏓 @dopenguin

🏓 @oeninghe-dataport

@oeninghe-dataport
oeninghe-dataport force-pushed the next branch 2 times, most recently from a353df3 to 6a19ea0 Compare July 17, 2026 08:49
As decided in a not-yet-published architectural decision, we usually do
not want to use watchers, but explicit mutations instead :-(
As decided in a not-yet-published architectural decision, we usually do
not want to use watchers, but explicit mutations instead :-(
@oeninghe-dataport

Copy link
Copy Markdown
Collaborator Author

🏓 @dopenguin

@dopenguin dopenguin left a comment

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.

Quite a lot of actions seem to fire when removing the plugin 🤨

Image

🏓 @oeninghe-dataport

Comment thread src/lib/watchArray.ts

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.

These functions are not in use. Please remove them.

Comment on lines 32 to 36
if (open.value === props.id) {
open.value = null
useCoreStore().setMoveHandle(null)
iconMenuStore.openMenuById(null)
} else {
open.value = props.id
iconMenuStore.openInMoveHandle(props.id)
iconMenuStore.openMenuById(props.id)
}

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.

Might as well just call iconMenuStore.openMenuById(open.value === props.id ? null : props.id)

Comment on lines 125 to 129
if (open.value === id) {
open.value = null
coreStore.setMoveHandle(null)
iconMenuStore.openMenuById(null)
} else {
open.value = id
iconMenuStore.openInMoveHandle(id)
iconMenuStore.openMenuById(id)
}

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.

Might as well just call iconMenuStore.openMenuById(open.value === id ? null : id)

*
* To change the open menu, use {@link openMenuById}.
*
* @readonly

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.

Suggested change
* @readonly
* @readonly
* @alpha

*
* To change the open focus menu, use {@link openFocusMenuById}.
*
* @readonly

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.

Suggested change
* @readonly
* @readonly
* @alpha

/**
* Appends a group of plugins to the icon menu.
*
* @param menu - The menu item group to add.

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.

Suggested change
* @param menu - The menu item group to add.
* @param menu - The menu item group to add.
* @alpha

/**
* Appends a plugin to the icon menu as a focus menu.
*
* @param menu - The focus menu item to add.

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.

Suggested change
* @param menu - The focus menu item to add.
* @param menu - The focus menu item to add.
* @alpha

/**
* Removes a plugin from the icon menu.
*
* @param pluginId - The ID of the plugin to remove.

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.

Suggested change
* @param pluginId - The ID of the plugin to remove.
* @param pluginId - The ID of the plugin to remove.
* @alpha

)
filteredMenuGroup.forEach(({ plugin }) => {
if (plugin.component) {
markRaw(plugin.component)

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 was previously more defensive with an additional toRaw. I feel like this should be readded here and in addFocusPlugin

Comment on lines +43 to +46
function isPluginInIconMenu(pluginId: string) {
const display = coreStore.configuration[pluginId]?.displayComponent
return typeof display === 'boolean' ? display : true
}

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.

This is not related to the intention of this PR thus 🎩, but I realized that this never did anything if the configuration was directly added to the plugin. If you see a quick solution, add it here, otherwise, I'll make a note for myself.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Plugins added by IconMenu can not be removed with "removePlugin"

2 participants