Skip to content
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.

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

Add settings docs #283

Merged
merged 2 commits into from
Apr 12, 2017
Merged

Add settings docs #283

merged 2 commits into from
Apr 12, 2017

Conversation

BrainMaestro
Copy link
Contributor

Finish last part of #230. Does this need to be more detailed?

Copy link
Collaborator

@KELiON KELiON left a comment

Choose a reason for hiding this comment

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

Thanks for adding it!

* `bool`
* `option`
* `array`
* `defaultValue` - `Any`, default value for the setting;
Copy link
Collaborator

Choose a reason for hiding this comment

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

  1. There is also label option to specify label text (it is just a field name by default)
  2. I'd notice that options and multi applicable only for array and option types
  3. For array and option types also could be added clearable and creatable options
  4. We can add screenshot to show how plugin settings look like
  5. We can add a link to react-select configuration to clarify what is creatable, multi, and clearable. Also, to clarify format of options object
  6. And the latest step is to add an example plugin with settings to examples.md

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  1. True
  2. multi should actually only be applicable to option. array probably shouldn't ever be restricted to just one value.
  3. The idea for array was to be the creatable version of option. I think they should all just be clearable by default, but that can be added.

I'll update the others

Copy link
Collaborator

Choose a reason for hiding this comment

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

@BrainMaestro agree with 2, but not with 3. I think it is possible case to select the list of provided options, without ability to add new ones, but at the same time it is possible to choose only one, but with ability to create it by user. What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay I think that's fine

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we no longer need the array type though. Since option can allow the user to create their own option. It should just handle all the possible scenarios.
What do you think?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@BrainMaestro makes sense! because type=option + multi will be the same as type=array. Let's just leave one

@BrainMaestro
Copy link
Contributor Author

I updated this with an example and removed the array type. I can make a separate PR to remove the array type from the codebase.

@KELiON
Copy link
Collaborator

KELiON commented Apr 12, 2017

👏👏👏

@KELiON KELiON merged commit aaa595a into cerebroapp:master Apr 12, 2017
@BrainMaestro BrainMaestro deleted the add-settings-docs branch April 12, 2017 09:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants