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

Enable loading of values for all 'select-type' components via Form input data #197

Closed
8 of 9 tasks
Tracked by #200
MaxTru opened this issue Nov 8, 2021 · 27 comments · Fixed by #270
Closed
8 of 9 tasks
Tracked by #200

Enable loading of values for all 'select-type' components via Form input data #197

MaxTru opened this issue Nov 8, 2021 · 27 comments · Fixed by #270
Assignees
Labels
channel:support enhancement New feature or request

Comments

@MaxTru
Copy link
Contributor

MaxTru commented Nov 8, 2021

Is your feature request related to a problem? Please describe

As part of our dynamic data loading initiative for our form components, we would want to enable users to drive their select-able data from sources other than the form schema.

As a starting point, we'll enable users to pass in the data themselves from outside the form via form input data, this first ticket handles that functionality. Following up to this will be allowing the configuration of the type of data source via the properties panel, as it is currently blocked by #249.

Describe the solution you'd like

We want to support the rendering of 'select-type' component values via Input data, so that a form designer may dynamically build up these components.

To this end, we'll need a few things:

  • Create a differentiation in the schema for different data sources
  • Allow our form components to interpret that differentiation and render appropriately
  • Figure out how we want these dynamically driven form components to look in the preview when no data source is provided

SCHEMA EXAMPLE

schema

{
  // values is ommited on purpose!
  //  "values": [
  //  {
  //   "label": "Value",
  //   "value": "value"
  //   }
  // ],
  "label": "Radio",
  "type": "radio",
  "id": "myRadio",
  "key": "radio",
  "valuesKey": "radioInputs"
}

form data input

{
  "radio": "valA",
  "radioInputs": [
    {
      "label": "valA",
      "value": "valA"
    },
    {
      "label": "valB",
      "value": "valB"
    },
  ]
}

Result
Screenshot_20211108_203747


Implementation steps:

Describe alternatives you've considered

n/a

Additional context

Requires #196
Raised during discussion of https://jira.camunda.com/browse/SUPPORT-12025
Also requested via https://jira.camunda.com/browse/SUPPORT-12306
Child of #200

@dfulgham
Copy link
Contributor

How about something like this
image

@bpmn-io-tasks bpmn-io-tasks bot removed the backlog Queued in backlog label Mar 14, 2022
@MaxTru MaxTru added the ready Ready to be worked on label Mar 14, 2022 — with bpmn-io-tasks
@MaxTru
Copy link
Contributor Author

MaxTru commented Mar 14, 2022

Hi @dfulgham ,

thanks for investing into this!

Just so that I understand:

  • You propose a new property (Options Key)
  • This Options Key will be used to pass an array of options
  • In addition one may use the regular key to supply the option that shall be selected by default I suppose?

Could you please elaborate why you propose to introduce a new property for this and not re-use the existing key (and just give it an array?) I suppose because you want to support the Select entry per default use-case. Is that right? Any other reasons?

@dfulgham
Copy link
Contributor

@MaxTru

  • You propose a new property (Options Key)
  • This Options Key will be used to pass an array of options
  • In addition one may use the regular key to supply the option that shall be selected by default I suppose?

Yes in all 3 points, as with all fields I assumed that its "key" would be used to set its existing value from imported data, and hold its selected value if changed, so this is why I elected to go with a new set of properties to select the data that would be used to populate the option values.

  • optionsKey will be the key matching the data property in the imported data, that will be used for options.
  • optionsFromKey is Boolean indicating if this feature is to be used, when true it reveals the optionsKey entry and removes the values group from the property panel, as to not confuse the user.
  • key will continue to be used to assign initial value from data, and hold the selected value.

Could you please elaborate why you propose to introduce a new property for this and not re-use the existing key (and just give it an array?) I suppose because you want to support the Select entry per default use-case. Is that right? Any other reasons?

To elaborate, yes in keeping with the function of all fields the key indicates which data is to be used for its value, no other reason. Supplying and Array of { label, value } to the fields key would not be aligned with existing function. If I were to use the fields key to pass an array, its initial value would be an array, then once I selected a value, that keys value would change to just that selected value, and then the select would no longer function, as the value would now be a primitive ( string, number, or boolean ).

It also has the added benefit of being able to re-use the same options on other fields (if needed), in the example I used the "options" key for both the select and the radio. Use case would be if you have a long standard set of options and there are a few fields where you would need to use them on a single form.

dfulgham added a commit to dfulgham/form-js that referenced this issue Mar 14, 2022
* Adding OptionsFromKeyEntry to GeneralGroup
* added optionFromKey:boolean, optionsKey:string field properties to allow select and radio to load their options form imported data.
* updated importer to import initialData for keys used by selects and radios
* Also refactored playground submit data to properly filter to only fields that are present.

Refs bpmn-io#197
@MaxTru MaxTru self-assigned this Mar 23, 2022
@MaxTru MaxTru removed their assignment Apr 5, 2022
@Skaiir
Copy link
Contributor

Skaiir commented Apr 22, 2022

@dfulgham Fair point from a usability perspective homogenizing input and output values is more intuitive 👍

@pinussilvestrus
Copy link
Contributor

pinussilvestrus commented Apr 29, 2022

As discussed via the dynamic forms Kickoff, we also plan to support providing functions next to a simple list for dynamic values.

A sketch of this, not necessarily what we are going to build

// Schema
{
  "components": [
    {
      "key": "product",
      "label": "Product",
      "type": "radio",
      "values": [],
      "valuesKey": "productValues",
      "id": "Field_0te6n1q"
    },
    ...
  ]
}

// Input data
{
  "product": "camunda-platform",
  "productValues": async function() { 

    // do whatever you want
    // return possible values

    const products = await fetch(...);
    
    return products.map(p => {
      return { label: p.name, value: p.key }
    }
  }
}

@bpmn-io-tasks bpmn-io-tasks bot added in progress Currently worked on and removed ready Ready to be worked on in progress Currently worked on labels May 2, 2022
@pinussilvestrus pinussilvestrus added the backlog Queued in backlog label May 2, 2022 — with bpmn-io-tasks
@pinussilvestrus
Copy link
Contributor

pinussilvestrus commented May 2, 2022

I used this pull request to create a spike.
Branch : https://github.com/bpmn-io/form-js/tree/197-dynamic-multi-components

  • properties panel: add a "valuesKey" property to specify a process variable to populate options (select, radio ...)
  • import: keep data that is specified via form field "valuesKey"
  • viewer: display options from "valuesKey" (select, radio, ...)
  • todo: allow functions to populate options
npx @bpmn-io/sr bpmn-io/form-js#197-dynamic-multi-components -c "npm run start:playground" --verbose

Kapture 2022-05-02 at 15 05 39

Observations / open questions (WIP)

general

  • Do we make a hard cut between static and dynamic options?
    • E.g. disable/remove "values" when "valuesKey" is specified vs. allow both and merge
  • Can we come up with a better name as "valuesKey"?

form-js-editor

  • Is the "valuesKey" input sufficient to mark dynamic population or do we need a dedicated checkbox to enable it (cf. this comment)?
  • What do we do with the "defaultValue" configuration? Does this make sense for the dynamic data use case?
  • What do we display in the editor "preview"? A placeholder? A hint that options are populated later?

form-js-viewer

  • On import: do we need to validate the options (array with value + label)?
  • Make the options population a generic concern (e.g. via useValuesKey hook)
  • Do we keep the "valuesKey" data on submit?

form-js-playground

  • With the increasing amount of input data, do we need resizable sections?
  • Do we allow providing functions in the data editors? Currently only JSON

@pinussilvestrus pinussilvestrus added the in progress Currently worked on label May 2, 2022 — with bpmn-io-tasks
@pinussilvestrus pinussilvestrus removed the backlog Queued in backlog label May 2, 2022
@MaxTru
Copy link
Contributor Author

MaxTru commented May 2, 2022

Very interesting questions you summarized here @pinussilvestrus !

My 2-cents (gut feeling):

  • I think the key question is: do we make a hard cut between static and dynamic values (as you stated)
  • From a user perspective, I think a hybrid use case does make sense (e.g., always have some default values present, add more values dynamically) and now exlcuding this design sounds like not a good idea to me
  • If we support the hybrid use case, then:
    • What do we do with the "defaultValue" configuration? Does this make sense for the dynamic data use case? => keep behavior as is. You can select a default value. But if data is mapped to the key, this overwrites the default value selected.
    • In the properties panel, I would explictly label the two section to input: Static values and Dynamic values key and would put these two very closely together
    • This would also remove the need for a dedicated checkBox obviously
    • What do we display in the editor "preview"? A placeholder? A hint that options are populated later? => I think no change is needed? We also don't show anything in the editor as of now? Do I miss something?
      Screenshot_20220502_172705

I have no insight or opinion on:

Do we keep the "valuesKey" data on submit?

Do we allow providing functions in the data editors? Currently only JSON

@nikku
Copy link
Member

nikku commented May 3, 2022

From a user perspective, I think a hybrid use case does make sense (e.g., always have some default values present, add more values dynamically) and now exlcuding this design sounds like not a good idea to me

I'd disagree here. What will be the composition semantics? How are default and dynamic values joined? Could I overrule default values, if so how? Hybrid does raise so many questions, both for the modeler, but also as I hook up dynamic data.


My two cents:

  • Let us keep things simple and clearly separate dynamic from static input
  • Consider populating default value dynamically, how would that work?

Static vs. dynamic is a classic case for me of "using expressions rather than UI". If it was a FEEL expression, static vs. dynamic is handled naturally:

# dynamic default value, sourced from `defaultValue` variable
= defaultValue

# static, hard coded default values
= [ { name: 'foo', value: 'bar' } ]

I know that we'll leave the realm of "simple editing" here. But the UI becomes significantly complex with modes, too.

If I baked it into the UI, I'd do so via a implementation select, cf.:

image


So what I'd suggest us doing is:

  • Settle down on Static vs. Dynamic (or allow hybrid) => I'd vote for clear separation
  • After we settled down clearly define how to incorporate these two modes into the UI.

Maybe it is even two different form components?


Simple answers 😉:

Do we keep the "valuesKey" data on submit?

No we don't we only keep data that is bound to a editable field. Cf. blog post.

Do we allow providing functions in the data editors? Currently only JSON

Not in the data editors ("playground"), but users can provide them as they integrate form-js.

@nikku
Copy link
Member

nikku commented May 3, 2022

What do we display in the editor "preview"? A placeholder? A hint that options are populated later?

Yes, that is what I'd do, at least for radio lists.

@nikku
Copy link
Member

nikku commented May 3, 2022

With the increasing amount of input data, do we need resizable sections?

I think so.

@Skaiir
Copy link
Contributor

Skaiir commented May 3, 2022

For my personal opinion:

Do we make a hard cut between static and dynamic options?

I think a hard cut makes it easier for now, we just need to support some kind of way in the future if it becomes apparent that the merging scenario is often used.

If we're ever allowing merging of sources this should be somewhat reflected in our UI/UX. If we ever intend to create sources like functions, will we allow those to be merged in as well? To me, a source of data is a source of data, whether they are static, input driven, generated, queried, ect... If we wanted a configuration level source concatenation, static and input driven should live on a flat plane together IMO.

Ultimately I think this is best left to FEEL,

Generally I'm just not a fan of two configuration areas to drive the same thing. Right now the way I'd leave it is to rewrite the values group in the properties panel to include a select field with all the different possible "sources" of data, and dynamically display the appropriate configuration fields depending on what you've selected. This is kinda nice because you don't have to worry about individual controls, the configuration is kept neatly within the "Values" block.

Can we come up with a better name as "valuesKey"?

Source something? Input something? Input Value Source 😄 I'm also not a big fan of ValuesKey either.

Is the "valuesKey" input sufficient to mark dynamic population or do we need a dedicated checkbox to enable it

I think it should be a select, we're going to have even more population sources in the future potentially. Then I'd imagine conditional fields that appear from the select.

Do we keep the "valuesKey" data on submit?

No. Any process should already be aware of enough context that that particular value shouldn't offer any extra information.

@MaxTru
Copy link
Contributor Author

MaxTru commented May 9, 2022

Alignment with @felix-mueller and @christian-konrad

  • Basic decision: we want to have the form-js improvements (dynamics forms incl. multi-element components, potentially also dynamic templates) also for C8 / C8 Tasklist
  • This will be released stable in the October release (C8.1)
  • To try this out early / provide to users early, we might use web modeler (cc. @CatalinaMoisuc) => e.g., load the latest form-js with the new features, if alpha version of taskList is used
  • ====> This means tasklist will implement this new feature for the new minor (on the 8.1 branch) cc. @vsgoulart

@dfulgham
Copy link
Contributor

dfulgham commented May 9, 2022

From a user perspective, I think a hybrid use case does make sense (e.g., always have some default values present, add more values dynamically) and now exlcuding this design sounds like not a good idea to me

I'd disagree here. What will be the composition semantics? How are default and dynamic values joined? Could I overrule default values, if so how? Hybrid does raise so many questions, both for the modeler, but also as I hook up dynamic data.

Sorry I've been away for a bit, back now. I agree with @nikku here if you wanted to implement a hybrid that could be accomplished within you own client code, you would have the schema available, to integrate the default values however you like and then modify the schema before substantiating the Form. Adding something like this within the component would not be recommended.

Also with the work on custom components you could create a component to handle this anyway you would like.

Also now that I'm back, what can I work on here to move this along?

@Skaiir Skaiir changed the title Provide values for multi-element components via Form data (input) Enable loading of values for all 'select-type' components via Form input data May 13, 2022
@Skaiir Skaiir assigned Skaiir and unassigned pinussilvestrus May 13, 2022
@Skaiir
Copy link
Contributor

Skaiir commented May 25, 2022

We had some discussions on whether we want to include a datasource property to explicitly dictate which source would be used as part of the schema instead of just assuming based on the implementation properties (for example values indicates that a static list is used, valuesKey indicates a dynamic data source ect.).

The main reason for this was to know how we'd handle the case where someone sets multiple of these properties.

What we'll do however instead is just agree on an order of defaulting priorities, and always make sure that no two properties are set within the implementation in form-js. This lets us be flexible without adding a whole new property. Hence the final format will be set as per the issue example.

@Skaiir Skaiir added the in progress Currently worked on label Jun 7, 2022 — with bpmn-io-tasks
@Skaiir Skaiir removed the ready Ready to be worked on label Jun 7, 2022
@Skaiir
Copy link
Contributor

Skaiir commented Jun 10, 2022

Couple of decisions I'd like to document on this for reference in the future:

  • Data will be loaded via an async ready hook that will make use of state to give information on how far the data is in the loading process. Basically, data will be loading - loaded or error out. This is to give us the flexibility in the future when we do add truly asynchronous ways of loading data to the form (via function or user-time loading).
  • We had a discussion on data coupling in form-js, and for things to be simple we're not going to make any changes here but it might be interesting to create a level of disconnection between the controls and their implementation, because as it stands right now all the form-js components are very much aware of the implementation which means they cannot be re-used. This is fine for now but if we ever decide to use component libraries this will need a lot of reworking. Not within the scope but worth looking out for.
  • Default values will be entirely disabled for fields which take options when those are not static. The simplest way to think about this is, if you're defining your options in the form schema, you'll want to define your defaults in the form schema. If you're building the options from the input variables, you can also pass your default from input variables (this functionality already exists, nothing is lost here). NOTE: Multi-selects will not be able to be defaulted yet, we need a properties panel friendly control for this we don't currently have (tracked here).
  • Most references to "values" have been renamed to "option" for the sanity of the developers. The only exception being the schema itself which will need a larger conversation before we do anything to it. I'll raise something in the future.

Expect a Draft PR for this very soon 😄

@nikku
Copy link
Member

nikku commented Jun 10, 2022

Data will be loaded via an async ready hook that will make use of state to give information on how far the data is in the loading process.

If this indicates "percentage progress wise" I'd argue we do not need it. Data loading (and capturing the three states "loading", "loaded" and "error" can be mapped to promises in a straight forward manner => We should simply offer a promise based interfaces.

Happy to checkout your draft PR, I'm looking forward to it.

@Skaiir
Copy link
Contributor

Skaiir commented Jun 10, 2022

@nikku Not intending to have percentage progress, but doing this in a stateful manner is quite nice within React because 1: it lets us do conditional rendering based on the state (picture a spinner based on the 'loading' state) and 2: it gives us the flexibility to mix and match based on our implementation. We can definitely discuss more how we want this to look but the way I see it there will need to be some kind of state somewhere, so might as well go full React with it. (useState/useEffect to drive the async stuff).

@nikku
Copy link
Member

nikku commented Jun 10, 2022

My point is: Let us not go "Full react" with interfaces to the outside we propose. Let us go "Web standards" and allow fetching data "The way users fetch data on the web".

Happy to continue the discourse on the PR. Looking forward.

@pinussilvestrus
Copy link
Contributor

My point is: Let us not go "Full react" with interfaces to the outside we propose. Let us go "Web standards" and allow fetching data "The way users fetch data on the web".

As I understood it, at this stage, we are not talking about outside API here but how we handle async data loading internally. But let's wait for a PR to discuss this manner 👍

Skaiir added a commit that referenced this issue Jul 1, 2022
@bpmn-io-tasks bpmn-io-tasks bot added needs review Review pending in progress Currently worked on and removed in progress Currently worked on needs review Review pending labels Jul 1, 2022
@tasso94
Copy link

tasso94 commented Jul 7, 2022

Hi all,

In the course of loosely defining what we need to do to add this feature to C7, we had an idea I would like to share.

The idea revolves around the use case that a developer wants to do rapid prototyping with this feature.

How the dynamic values are defined in JSON makes sense for real-world scenarios where you want to assign a proper label to an element. However, it stands a bit in the way of "quickly trying something out". For the "rapid prototyping" use case, we thought we could additionally offer a mapping that would shorten the whole thing a bit:

["dynamicValue1", "dynamicValue2"]

Would be mapped to:

[
  {
    "label": "dynamicValue1",
    "value": "dynamicValue1"
  },
  {
    "label": "dynamicValue2",
    "value": "dynamicValue2"
  }
]

Both label and value would be assigned the same value. That's not nice for real-world use cases. However, it might be just right if you want to "try something out quickly".

Do you have an opinion on that? We can easily build it ourselves in C7, but I wanted to share the idea with a broader group and get some more thoughts.

Best,
Tassilo

@pinussilvestrus
Copy link
Contributor

Hi @tasso94, pretty interesting thought, something like a "quick-edit" of input data. I could imagine this not only for the JSON data but also for defining values in the schema (aka "static values").

{
  "label": "Radio",
  "type": "radio",
  "id": "myRadio",
  "key": "radio",
  "values": [ "Value1", "Value2", "Value3" ]
}

This would definitely ease the way how you provide values, in case you need it. However, we would also need to maintain this syntax, e.g.: Would the output data then also have this short format, or would it be transformed into the label-value syntax? Can I configure this?

To not overload this issue, as it's already shaped and in progress, can you maybe open another one where we can discuss this matter? That would be helpful 🙂

/cc @nikku @christian-konrad @Skaiir @CatalinaMoisuc

@Skaiir
Copy link
Contributor

Skaiir commented Jul 7, 2022

I actually had this idea in mind but somewhere along the line I forgot about it. Honestly, nothing stops us from handling this case, and it wouldn't be a big dev effort. I second @pinussilvestrus though we can take this discussion to another issue.

@pinussilvestrus pinussilvestrus added the needs review Review pending label Jul 15, 2022 — with bpmn-io-tasks
@pinussilvestrus pinussilvestrus removed the in progress Currently worked on label Jul 15, 2022
Skaiir added a commit that referenced this issue Jul 18, 2022
@fake-join fake-join bot closed this as completed in #270 Jul 18, 2022
@bpmn-io-tasks bpmn-io-tasks bot removed the needs review Review pending label Jul 18, 2022
fake-join bot pushed a commit that referenced this issue Jul 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
channel:support enhancement New feature or request
Projects
None yet
8 participants