Allow removal and adding for plugins in iconMenu - #861
oeninghe-dataport wants to merge 21 commits into
Conversation
|
a353df3 to
6a19ea0
Compare
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 :-(
| function isPluginInIconMenu(pluginId: string) { | ||
| const display = coreStore.configuration[pluginId]?.displayComponent | ||
| return typeof display === 'boolean' ? display : true | ||
| } |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I do not get why this check is necessary at all. I kept it here to prohibit regression, but in what scenario I'd add a plugin to the iconMenu (not standalone) but then do not display it?
There was a problem hiding this comment.
So, if you agree, I'll remove this check
There was a problem hiding this comment.
I honestly thought that both layoutTag and displayComponent both don't do anything for plugins added to the iconMenu. Please add this to our next meeting agenda so we can think about dropping this.
Most clients already handle the use case this would tackle by simply not adding them then.
There was a problem hiding this comment.
As discussed in-person, I removed the check: c1552fb
Co-authored-by: Pascal Röhling <73653210+dopenguin@users.noreply.github.com>
Co-authored-by: Pascal Röhling <73653210+dopenguin@users.noreply.github.com>
Co-authored-by: Pascal Röhling <73653210+dopenguin@users.noreply.github.com>
Co-authored-by: Pascal Röhling <73653210+dopenguin@users.noreply.github.com>
Co-authored-by: Pascal Röhling <73653210+dopenguin@users.noreply.github.com>
|
Please take a look at the amount of actions mentioned in #861 (review) |
a77cdd7 to
7bbdec2
Compare
The actions were there for several reasons. I reduced them a lot with 7bbdec2 |
| function getPluginStore<T extends BundledPluginId>( | ||
| id: T | ||
| ): ReturnType< | ||
| T extends BundledPluginId | ||
| ? BundledPluginStores<typeof id> | ||
| : PolarPluginStore | ||
| > | null { | ||
| ): ReturnType<BundledPluginStores<T>> | null |
There was a problem hiding this comment.
Wasn't it the intention of the function to also be able to retrieve the store of custom plugins with this function?
There was a problem hiding this comment.
It is; see line 73
|
|
||
| return { | ||
| plugins, | ||
| plugins: readonly(plugins) as readonly PluginContainer[], |
There was a problem hiding this comment.
Is this the same as using a computed?
There was a problem hiding this comment.
It is...well it is mostly. readonly is deep. The exact equivalent to computed(() => plugins.value) would probably be shallowReadonly(plugins)
| if (pluginIndex !== -1) { | ||
| // eslint-disable-next-line @typescript-eslint/no-non-null-assertion | ||
| const pluginMenu = menus.value[pluginIndex]! |
There was a problem hiding this comment.
Simply moving pluginMenu outside the if-statement and then checking whether pluginMenu works even better.
|
@oeninghe-dataport I'll take a look after you updated the PR |
This branch has not been deployed
2 participants
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
Relevant tickets, issues, et cetera
Fixes #668