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

WIP/POC - Enable declarative way of panel options / editors definition #16731

Closed
wants to merge 7 commits into from

Conversation

dprokop
Copy link
Member

@dprokop dprokop commented Apr 23, 2019

Based on #16718

This is a heavy POC/WIP

The idea behind this PR is to enable declarative way of panel editors definition. Core typings are available in panelOptions.ts. Based on options UI model the PanelOptionsUIBuilder component will create options editor. For now there are only simple option inputs implemented: BooleanOption, Numeric(Integer/Float)Option.

Major proposed change is OptionInputAPI interface that would enforce all option editor components to have a common API (value/onChange). Some steps in this direction are already taken (see ThresholdsEditor, ValueMappingsEditor).

To see the proposed JSON model for options UI take a look at panel/gauge/module.tsx. Gauge panel options editor is now rendered from options JSON model using a very primitive (yet) PanelOptionsUIBuilder component

Next step is to implement and API to make the model creation easy. The draft shape will be sth more or less like:

 const MyPanelOptionModel = new OptionsEditorUIModel<PanelOptions>()
   .addRow(/*{row config, i.e. number of column }*/)
     .addColumn()
       .addPanel('Basic settings') // Creates
         .useStatsPicker('Show', 'stats')
         .useUnitPicker('Unit', 'unit')
         .useDecimalsInput('Decimals', 'decimals')
         .useTextInput('Prefix', 'prefix')
         .useTextInput('Suffix', 'suffix')
         .addSelectOption('Font size', 'fontSize', /*[...options]*/ )
       .endPanel()
     .endColumn()
     .addColumn()
       .addPanel('Gauge')
         .useNumber('Min value', 'minValue')
         .useNumber('Max value', 'maxValue')
         .useToggle('Show labels', 'showThresholdLabels')
         .useToggle('Show markers', 'showThresholdMarkers')
       .endPanel()
     .endColumn()
     .addColumn()
       .addPanel('Threshold')
         .useThresholdEditor('thresholds')
       .endPanel()
     .endColumn()
   .endRow();

TODO:

  • Implement basic options UI elements (i.e. TextOption, SelectOption...)
  • Implement API for options UI model creation
  • Implement PanelOptionsUIBuilder components to render options edito layout/panels/etc
  • Make it panel independent (no Panel in naming convention)

@dprokop dprokop requested review from torkelo and ryantxu April 23, 2019 18:59
@ryantxu
Copy link
Member

ryantxu commented Apr 23, 2019

Quick feedback without having looked far...

  1. love it :)
  2. Lets name things to feel appropriate for more than just panels -- this same stuff would be useful in datasources, apps and some builtin UI elements.

Rather than PanelOptions maybe OptionsEditor

I'll look more later

@dprokop
Copy link
Member Author

dprokop commented Apr 23, 2019

Rather than PanelOptions maybe OptionsEditor

Definitely. For now I focused more on the typings, so would love to get some feedback on that:)

@torkelo
Copy link
Member

torkelo commented Apr 23, 2019

Cool! yea, the addPanel and using the Panel name for a "options box" feels confusing as Panel term is used already :)

* it could be 'minValue'
* Question: Should we name it option instead of path?
*/
path: P;
Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure we should name it path since I'm allowing shallow options manipulation only. I think we should strive for editors composition(via declarative API, i.e. useCustomOption('Some custom options', 'customOptionKey, CustomOptionEditor)` rather than handling deep paths for options. Any thoughts on that @ryantxu?

Copy link
Member

Choose a reason for hiding this comment

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

Does path represent the json attribute name? if so, maybe property? and make sure it is a valid property

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, it's basically json attribute name. property sgtm

Boolean = 'boolean',
Object = 'object',
Array = 'array',
}
Copy link
Member

Choose a reason for hiding this comment

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

I don't fully understand how this will work, but Array strikes me as a differnt camp. I would expect somethign like:
DataType

  • text
  • float
  • integer
  • boolean
  • custom (=> object)

Properties:

  • multi-valued (stored as array)
  • min/max/step (numbers)
  • MaxLength (strings)
    etc

Copy link
Member Author

Choose a reason for hiding this comment

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

@ryantxu it is not used yet. Array/Object will not be there for sure. Thing is I wanted to use it for primitive option types to automatically select UI component based on the option type, if no component was specified. That's why component is an optional prop in OptionUIModel interface

* + Option 2 on/off +
* + ... +
* ++++++++++++++++++++++++++++++++++++++
*/
Copy link
Member

Choose a reason for hiding this comment

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

I love ascii art comments ❤️

// // .useTextInput('Suffix', 'valueMappings.suffix')
// // .addBooleanOption('Title', 'option.path')
// // .addSelectOption('Title', 'option.path', )
// // .endPanel()
Copy link
Member

Choose a reason for hiding this comment

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

What are thoughts on how an external plugin would add to this?

Copy link
Member

Choose a reason for hiding this comment

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

there will be a way to add custom components of course. Adding high level structures (groups) that can be reused will be a bit trickier :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I was thinking about it and my idea was to stick with the row/column approach, but enable used component config, i.e. addRow({renderAs: MyRowComponent}). Same with groupings: for now I thought only on panel(which name we need to change) and fiieldsets which are panel options specific. But the same approach could be taken towards grouping, i.e. current addPanel and addFieldset could use more generic function addGroup({renderAs: PanelOptionsGroup}).

Copy link
Member

@ryantxu ryantxu left a comment

Choose a reason for hiding this comment

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

This is really important and I am happy it is getting attention. This will help a lot.

My other big reaction/advice is to make sure we check existing solutions for inspiration and see if they are appropriate to use/extend. The biggest one that pops out in my google searching is:
https://mozilla-services.github.io/react-jsonschema-form/

Maybe also check:
https://jsonforms.io/
https://medium.freecodecamp.org/how-to-autogenerate-forms-in-react-and-material-ui-with-mson-5771b1b7e739
https://github.com/redgeoff/mson

The advantage of starting with something like JSONSchema is that we get robust validation out-of-the-box. The disadvantage is that is is json :) and would be better as typescript. Quick googling show: https://github.com/bcherny/json-schema-to-typescript -- here is an article that explores some options: https://spin.atomicobject.com/2018/03/26/typescript-data-validation/

@dprokop
Copy link
Member Author

dprokop commented May 6, 2019

Closing in favour of #16773

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.

3 participants