Add plugin settings #230

BrainMaestro · 2017-03-10T13:10:40Z

This is to implement #222

Currently it looks like this

There's support for strings, number, array, options as different settings types. Each setting can also include a description and default value.

Add plugin settings for those that specify on startup
Render plugin settings in settings UI
Save changed plugin settings
Provide access to specified settings to plugins. i.e something similar to term or display that is passed in to a plugin's function
Add support for default values
Update plugin documentation

It's still a work in progress, but I didn't feel comfortable doing so much without showing you. Just in case you don't agree with the way it is implemented or have other suggestions.

Fixes #222

KELiON · 2017-03-10T15:45:36Z

@BrainMaestro this is awesome! Great job! 👏 👏

BrainMaestro · 2017-03-10T16:48:58Z

Thanks!. I completely missed the lint issues, but I'll fix them later. Should I go ahead?

Also, I noticed a bug with the settings plugin. It does not rerender settings that were changed as soon they were changed. Any idea why? This bug was not introduced by my changes.

BrainMaestro · 2017-03-10T17:50:04Z

This feature is ready for review now

KELiON

It looks great! Check out my comments and let me know if you think it is ok to fix them

KELiON · 2017-03-13T08:16:04Z

app/main/actions/search.js

+    const settings = externalSettings[name]
+    if (settings) {
+      Object.keys(settings).forEach(key => {
+        settings[key] = settings[key].value


Why don't you store this settings as key=>value?

You mean having the setting name at the top level instead of all of them nested under external?

@BrainMaestro no, I mean why do you need nested object with value => '...'? Now externalSettings is something like:

{ pluginName: { language: { value: 'en' }, token: { value: 'user-token' } } }

But it could be simplified as:

{ pluginName: { language: 'en', token: 'user-token' } }

Yes that's better, but for each setting we still need to keep a type, description, defaultValue and other specific settings like multi and options. So the actual structure is closer to:

{ pluginName: { language: { type: 'option', description: 'Your preferred language', options: ['en', 'fr'], value: 'en' }, token: { type: 'string', value: 'user-token' } } }

KELiON · 2017-03-13T08:17:08Z

app/main/plugins/core/cerebro/settings/Settings/components/checkbox.js

+)
+
+Checkbox.propTypes = {
+  value: PropTypes.any,


Do you see any cases to have non-boolean value here?

Mistake from copying proptypes over from other components

KELiON · 2017-03-13T08:19:09Z

app/main/plugins/core/cerebro/settings/Settings/components/input.js

@@ -0,0 +1,30 @@
+import React, { PropTypes } from 'react'
+import styles from '../styles.css'
+import hotkeyStyles from '../Hotkey/styles.css'


I think it makes sense to move hotkey into components directory and reuse styles for this component for all text inputs (so, we won't have separate css file for Hotkey component)

KELiON · 2017-03-13T08:28:24Z

app/main/plugins/core/cerebro/settings/Settings/external-settings.js

+        key={label}
+        label={label}
+        value={value}
+        onChange={({ target }) => this.changeSetting(plugin, label, target.value)}


Is it possible to change components a bit so all of them have the same API and onChange function always called with new value, instead of Event or value? In this case we can do something like this:

const components = { bool: Checkbox, option: Options, array: Select, } // ... const { type, value, defaultValue, ...restProps } = setting const FormComponent = components[type] || Input return <FormComponent value={value || defaultValue} key={label} label={label} onChange={newValue => this.changeSetting(plugin, label, newValue)} // ↓ this could contain `multi`, `options` or other specific fields ...restProps }

That's excellent!

KELiON · 2017-03-13T14:42:58Z

Also I've just tested it locally: we should provide default value for external key in config, because I don't have it in my config.json and rendering of settings raises an exception:

To do it you can just add external: {} to config.js file

KELiON · 2017-03-13T17:08:46Z

Any idea why? This bug was not introduced by my changes.

After git bisect I found that issue was introduced when we switched to yarn instead of npm. I'll try to investigate, but I bet the problem in some package update (probably react)

KELiON · 2017-03-13T17:20:09Z

Ok #238 should fix this issue

BrainMaestro · 2017-03-14T11:55:47Z

Should be good now

footnoise · 2017-03-16T12:06:51Z

@BrainMaestro @KELiON where can you guys merge it to master. I need this settings extension ASAP :-)

KELiON · 2017-03-16T17:57:37Z

@BrainMaestro I've changed few small things in branch feature/plugin-settings, I've opened PR in your repo so you can review progress there. Main thing – I've changed structure of objects that we store in config: we can remove duplication of settings definition and always get it from plugin object, and store in config only real values.

Also, now I'm thinking about moving plugin settings from Settings to plugin preview. So, if some plugin is installed – when you type plugins name and you see preview of this plugin, below description we can show its settings. What do you think? I'd also add there one general checkbox for all plugins – disable.

BrainMaestro · 2017-03-17T08:01:02Z

The PR was great! I'm not sure I understand your suggestion. You want to show the settings after the plugin preview? or as a separate entry in the search view under the plugin? The general disable checkbox is a great idea.

KELiON · 2017-03-17T08:41:53Z

@BrainMaestro when we search for plugins and focus on installed plugin, we can show settings above uninstall/details buttons.

Somethin similar Atom do, but they have separate button for settings and show them above Readme:

BrainMaestro · 2017-03-17T08:45:43Z

Oh. Yeah I like that idea. Do you want to implement it? or should I?

KELiON · 2017-03-17T08:48:39Z

@BrainMaestro I'm not sure I'll be able to do it until Monday, so feel free to implement it! We should also add information about settings to documentation and we're ready to go.

BrainMaestro · 2017-03-17T09:11:48Z

Alright. I'll take a look. Any idea why the tests are failing?

KELiON · 2017-03-20T09:46:17Z

@BrainMaestro did you have any time to fix it? If not – I can do it on this week. Regarding tests – looks weird, I'll check it later, but I think it is related to test webpack config and target option there

BrainMaestro · 2017-03-20T10:49:49Z

Yeah I'm working on it. Quite a few things are broken though, so it's taking longer than I thought. It should be done by the end of the day.

BrainMaestro · 2017-03-21T09:58:54Z

@KELiON I made the change to move the settings to the plugin view. Looks like this now:

Any style changes?

Also, I was thinking about throttling the onChange function for the Input settings component. So every keystroke doesn't get saved. Would that make sense?

KELiON · 2017-03-21T10:37:50Z

@BrainMaestro I think it looks good enough! Regarding throttling – I'd leave basic functionality for now. We can add it later if we find any problems

BrainMaestro · 2017-03-21T11:05:37Z

Alright. I'll add the settings documentation later, but I guess this is good to merge now?

KELiON · 2017-03-21T14:51:39Z

@BrainMaestro going to test it later today. Looks awesome!

KELiON · 2017-04-03T14:01:20Z

@BrainMaestro Hey! I hope in this PR we have latest changes related to settings and can finally release it. Can you review my changes there?

BrainMaestro · 2017-04-06T09:50:47Z

I left a comment there a few days ago. Not sure if you've seen it.

KELiON · 2017-04-06T09:53:30Z

@BrainMaestro I don't see any comments there

BrainMaestro · 2017-04-06T10:07:00Z

Oh I added it as a review comment by mistake. Should be there now.

Do not store settings structure in config, always get it from plugin object

BrainMaestro · 2017-04-06T16:47:06Z

Looks great

KELiON · 2017-04-07T15:28:53Z

Ok, now we only need to add it to documentation! Thank you @BrainMaestro for this big step!:)

KELiON requested changes Mar 13, 2017

View reviewed changes

BrainMaestro force-pushed the add-plugin-settings branch from ac89596 to 558914f Compare March 14, 2017 11:53

KELiON mentioned this pull request Mar 20, 2017

Windows icons support #198

Merged

BrainMaestro force-pushed the add-plugin-settings branch from 0be4992 to 4e900b7 Compare March 21, 2017 09:55

BrainMaestro force-pushed the add-plugin-settings branch 2 times, most recently from 8da7b49 to 6c7817d Compare April 6, 2017 16:42

BrainMaestro and others added 16 commits April 6, 2017 17:43

Add settings support for plugins

edca8f6

Render plugin settings

c2baf0b

Save changed external settings

715831c

Make plugin settings available to the plugin

9cb7453

Show default value if value is not present

f9b4277

Fix lint errors

1734b0f

Simplify external settings components rendering

e1350c7

Move styles to settings stylesheet

3558225

Make description required

7daf785

Changes in plugin settings (#1)

a17562a

Do not store settings structure in config, always get it from plugin object

Save settings to config

6dd1f56

Move plugin settings to plugins component preview

3235a70

Removed add method from plugin settings

f3fc172

Refactor plugin settings

d77edc3

Fix null/undefined value in select

57557c1

Remove add settings call

7d644e0

BrainMaestro force-pushed the add-plugin-settings branch from 6c7817d to 7d644e0 Compare April 6, 2017 16:45

KELiON merged commit 19c50bb into cerebroapp:master Apr 7, 2017

BrainMaestro deleted the add-plugin-settings branch April 7, 2017 16:03

BrainMaestro mentioned this pull request Apr 11, 2017

Add settings docs #283

Merged

Add plugin settings #230

Add plugin settings #230

Conversation

BrainMaestro commented Mar 10, 2017 • edited Loading

KELiON commented Mar 10, 2017 • edited Loading

BrainMaestro commented Mar 10, 2017

BrainMaestro commented Mar 10, 2017

KELiON left a comment

Choose a reason for hiding this comment

KELiON Mar 13, 2017

Choose a reason for hiding this comment

BrainMaestro Mar 13, 2017

Choose a reason for hiding this comment

KELiON Mar 14, 2017 • edited Loading

Choose a reason for hiding this comment

BrainMaestro Mar 14, 2017 • edited Loading

Choose a reason for hiding this comment

KELiON Mar 13, 2017

Choose a reason for hiding this comment

BrainMaestro Mar 13, 2017

Choose a reason for hiding this comment

KELiON Mar 13, 2017

Choose a reason for hiding this comment

KELiON Mar 13, 2017 • edited Loading

Choose a reason for hiding this comment

BrainMaestro Mar 13, 2017

Choose a reason for hiding this comment

KELiON commented Mar 13, 2017 • edited Loading

KELiON commented Mar 13, 2017

KELiON commented Mar 13, 2017

BrainMaestro commented Mar 14, 2017

footnoise commented Mar 16, 2017

KELiON commented Mar 16, 2017 • edited Loading

BrainMaestro commented Mar 17, 2017

KELiON commented Mar 17, 2017

BrainMaestro commented Mar 17, 2017

KELiON commented Mar 17, 2017

BrainMaestro commented Mar 17, 2017

KELiON commented Mar 20, 2017

BrainMaestro commented Mar 20, 2017

BrainMaestro commented Mar 21, 2017 • edited Loading

KELiON commented Mar 21, 2017

BrainMaestro commented Mar 21, 2017

KELiON commented Mar 21, 2017

KELiON commented Apr 3, 2017

BrainMaestro commented Apr 6, 2017

KELiON commented Apr 6, 2017

BrainMaestro commented Apr 6, 2017

BrainMaestro commented Apr 6, 2017

KELiON commented Apr 7, 2017

BrainMaestro commented Mar 10, 2017 •

edited

Loading

KELiON commented Mar 10, 2017 •

edited

Loading

KELiON Mar 14, 2017 •

edited

Loading

BrainMaestro Mar 14, 2017 •

edited

Loading

KELiON Mar 13, 2017 •

edited

Loading

KELiON commented Mar 13, 2017 •

edited

Loading

KELiON commented Mar 16, 2017 •

edited

Loading

BrainMaestro commented Mar 21, 2017 •

edited

Loading