Desktop: Resolves #6143: Add Installed Plugins in "About Joplin" #6357
Conversation
packages/lib/versionInfo.test.ts
Outdated
|
|
||
| describe('versionInfo', function() { | ||
|
|
||
| it('should return an array', (()=>{ |
There was a problem hiding this comment.
If you add return type annotation to getPluginsArr then this test becomes useless
There was a problem hiding this comment.
I will remove the test and add types to the function
packages/lib/versionInfo.test.ts
Outdated
| expect(plugins.length).toBe(0); | ||
| })); | ||
|
|
||
| it('should not push plugin to body if no plugin is found',(()=>{ |
There was a problem hiding this comment.
This test is not really doing anything meaningful as it never calls any code that is used by Joplin.
There was a problem hiding this comment.
Can you suggest any test case that I should Add?
I am having confusion about testing the file without any plugins in the test environment.
There was a problem hiding this comment.
Essentially you have a function from Plugins to string - this is all you need to test. When running in Joplin Plugins will be provided by the PluginService but in your test you can set the value manually and use it for testing.
packages/lib/versionInfo.test.ts
Outdated
|
|
||
| describe('versionInfo', function() { | ||
|
|
||
| it('should not push plugin to body if no plugin is found',() => { |
There was a problem hiding this comment.
I don't understand what this test is doing.
If it's checking getPluginsArr then why do you need the rest remaining code ('if' and 'map')? If it's checking the logic inside the if then what's point of checking it when this code is only used here in this test?
There was a problem hiding this comment.
Fair point, I'll remove that too.
| @@ -32,6 +52,14 @@ export default function versionInfo(packageInfo: any) { | |||
| console.info(gitInfo); | |||
| } | |||
|
|
|||
| const plugins = getPluginsArr(PluginService.instance().plugins); | |||
| if (plugins.length > 0) { | |||
There was a problem hiding this comment.
why not just move this code inside the getPluginsArr function?
There was a problem hiding this comment.
The gitInfo push to the body was down there so I thought to leave same types of items at a similar palace, also getPluginsArr just take the obj and return the array we need.
Should I change it?
packages/lib/versionInfo.ts
Outdated
| @@ -32,6 +52,14 @@ export default function versionInfo(packageInfo: any) { | |||
| console.info(gitInfo); | |||
| } | |||
|
|
|||
| const plugins = getPluginsArr(PluginService.instance().plugins); | |||
There was a problem hiding this comment.
From where versionInfo() is called, you already have the list of plugins in props.plugins, so you don't need to load the PluginService here. Just pass props.plugins to this function.
There was a problem hiding this comment.
props.plugins returns this with type of PluginState:
| }); | ||
| } | ||
| return plugins; | ||
| }; |
There was a problem hiding this comment.
I don't understand the purpose of this function. You're looping on the array to create a new array. Then later you loop on this new array to create the plugin strings. So why not eliminate this intermediate step and loop on the first array to directly create the plugin strings?
There was a problem hiding this comment.
PluginService.instance().plugins gives an object(with all plugins also as object) with id as the key, so I converted it to an array of objects and fill the array with only the name and version of the plugin.
| expect(plugins[1].version).toBe('0.2.1'); | ||
| }); | ||
|
|
||
| }); |
There was a problem hiding this comment.
None of this feels very useful. getPluginsArr is just a "for" loop and we assume that loops work. What you should test instead is the versionInfo() function - make sure that when there are plugins, it includes them in the output of the function.
There was a problem hiding this comment.
Cool, I'll add it.
|
Sorry I'm not very keen on merging this. |
|
Any specific reason. Should I have to add something else? |
This PR resolves #6143
I didn't Add the 20 Limit cos the Menu itself adds a scrollbar when it.
For reference, I added some screenshots with 50 plugins.
Mac:
Windows:
