Add conditional rendering #403

barmac · 2022-11-14T12:53:10Z

This PR implements conditional rendering via a Field#conditional.hide property. Condition needs to be a FEEL unary test which is parsed and evaluated by feelin. It is defined via the properties panel. Form designer can use any variables passed to the form object to define the condition.

Closes #374
Closes #401
Closes #415

Follow-up issues:

As a playground user, I want to get autocompletion for external data when defining condition #402

barmac · 2022-11-14T12:54:31Z

@nikku FEEL free to provide your feedback :)

barmac · 2022-11-14T12:54:46Z

Try this out via npm run start:playground.

packages/form-js-viewer/src/Form.js

barmac · 2022-11-15T13:31:06Z

packages/form-js-viewer/test/spec/condition.json

+      "type": "textfield",
+      "id": "Field",
+      "key": "text",
+      "condition": "=amount > 2"


Consider renaming.

christian-konrad · 2022-11-15T16:46:53Z

packages/form-js-viewer/package.json

@@ -41,6 +41,7 @@
    "@bpmn-io/snarkdown": "^2.1.0",
    "classnames": "^2.3.1",
    "didi": "^9.0.0",
+    "feelin": "^0.41.0",


We haven't moved it out of @nikku'S repo/org, right? Should we do it?

Camunda can consider to fork it (i.e. publish via @bpmn-io/feelin or the like). In this case it is likely the core modeling team to take responsibility over it.

But at this time I don't see it as strictly being required.

We can do that as soon as we want to implement something in the library.

christian-konrad · 2022-11-15T16:49:31Z

packages/form-js-viewer/src/core/ConditionChecker.js

+    }
+
+    try {
+      const result = unaryTest(condition.slice(1), data);


Someone reading this may not know that you slice the first char to remove the "="

I added a comment.

nikku · 2022-11-16T07:37:38Z

packages/form-js-playground/rollup.config.js

+
+// TODO(@barmac): remove once https://github.com/moment/luxon/issues/193 is resolved
+function onwarn(warning, warn) {


Sure that we want to ignore this? Can the built bundle still be consumed (conditions be evaluated)?

==> Let's add an integration test for this in form-js.

What would you expect to test? Something like 3c06472 ?

nikku · 2022-11-16T07:39:12Z

packages/form-js-viewer/src/core/ConditionChecker.js

+
+      return result;
+    } catch (error) {
+      return false;


Consider not swallowing the error, but logging it (or emitting it via eventBus.fire(error, ...).

Implemented via 848b703

nikku · 2022-11-16T07:39:43Z

packages/form-js-viewer/src/core/ConditionChecker.js

+    }
+
+    try {
+      const result = unaryTest(condition.slice(1), data);


I guess at this point we don't want to enable context sensitive parsing, right? Our inputs don't contain spaces and other special chars.

Not sure what you mean. I need to pass data in order to correctly evaluate the condition, right?

nikku · 2022-11-16T07:40:07Z

packages/form-js-viewer/src/core/ConditionChecker.js

+import { unaryTest } from 'feelin';
+import { isString } from 'min-dash';
+
+export class ConditionChecker {


Awesome that this is pluggable. ❤️

nikku · 2022-11-16T07:41:25Z

packages/form-js-viewer/src/render/hooks/useCondition.js

+
+/**
+ *
+ * @param {{ expression?: string } | undefined} condition


What is the result of this hook?

Return type is missing; unclear if it returns "true" to show or to hide.

I added documentation.

nikku

Great addition (service + hook). Just found some nit-picks you may act upon.

barmac · 2022-11-16T08:24:43Z

Thanks, I will address the comments tomorrow :)

pinussilvestrus · 2022-11-17T08:38:08Z

packages/form-js-editor/src/features/properties-panel/entries/ConditionEntry.js

+function useVariables() {
+  const form = useService('formEditor');
+  const schema = form.getSchema();
+
+  const variablesRef = useRef([]);
+
+  const variables = getSchemaVariables(schema);
+  const previousVariables = usePrevious(variables) || [];
+
+  const variablesChanged = !areEqual(variables, previousVariables);
+
+  if (variablesChanged) {
+    variablesRef.current = variables.map(key => ({ name: key }));
+  }
+
+  return variablesRef.current;
+}


Should we move this under /hooks? I can imagine we will need this more often in the future (similar to useCondition in the viewer)

Hmm I think I could simplify this a bit...

OK it's now simplified and exported from hooks.

I think we have to be careful here. The useService hook that is used in the new useVariables hook now needs a FormEditorContext to work correctly. Note that the properties panel can also be rendered outside of the editor. We use our own useService therefore: https://github.com/bpmn-io/form-js/blob/develop/packages/form-js-editor/src/features/properties-panel/hooks/usePropertiesPanelService.js

We either need the formEditor to be provided as a parameter for this hook or, alternatively, move the useVariables to the properties panels hooks.

Yeah this is probably why the tests started to fail after the change.

OK it should be fine now.

pinussilvestrus · 2022-11-17T09:12:57Z

packages/form-js-viewer/src/core/ConditionChecker.js

+   */
+  check(condition, data = {}) {
+    if (!condition) {
+      return true;


Should we allow to specify this defaultValue? I could imagine other cases where we do not default a condition to true, for example: readonly => default false.

This sounds to me more like a form field concern: it knows whether it should display or not.

Condition checker can be applied to anything, not only fields.

barmac · 2022-11-17T15:47:50Z

Based on https://github.com/camunda/product-hub/issues/56#issuecomment-1315173662, I will need to reverse the logic so that we use a "hideIf" field. This will also require changes in the properties panel. Let's do that in a separate issue.

Skaiir

Everything looks quite good, just got a few cleanup suggestions :).

Skaiir · 2022-11-16T07:58:13Z

packages/form-js-viewer/src/Form.js

@@ -170,23 +170,24 @@ export default class Form {
    const data = this._getSubmitData();

    const errors = this.validate();
+    const filteredErrors = this._applyConditions(errors, data);


I think we should filter this upstream. It seems strange to me that we are still validating the fields which are hidden via condition, as I believe the form should just treat them as non-existent.

Skaiir · 2022-11-16T08:00:34Z

packages/form-js-viewer/src/Form.js


      return {
        ...data,
        [ _path[ 0 ] ]: value
      };
    }, {});
+
+    const filteredValues = this._applyConditions(rawValues, { ...initialData, ...rawValues });


Again here, we're doing all the heavy lifting and filtering after the fact. I believe we should start filtering at the formField level. The way I would see it would be filterSomeHow(formFieldRegistry.getAll()). Or maybe even formFieldRegistry.getVisible().

Skaiir · 2022-11-16T08:16:01Z

packages/form-js-viewer/test/spec/render/components/FormField.spec.js

+    });
+
+
+    it('should use give precedence to form data to check condition', function() {


Skaiir · 2022-11-18T05:09:49Z

@barmac I'm not sure we should do the reversing in a separate issue. If I understand correctly you'd want to commit the current state once cleaned up to the develop and then in another commit invert the behavior right?

The problem with that is that we should really avoid changes to the form schema, especially if we know they are going to be temporary and if they break backwards compatibility.

I understand we don't want to be blocking this feature, but since this would break the backward compatibility of the schema, we'll still be blocked in practice from releasing until the inversion has been committed to develop.

barmac · 2022-11-18T10:45:41Z

Thanks for your feedback. It feels like we should have a conversation about this before the PR is merged. I'll set this up.

christian-konrad · 2022-11-23T16:57:57Z

@barmac can't run it in the playground:

Bildschirmaufnahme.2022-11-23.um.17.56.42.mov

Condition is a FEEL unary test which can use any values passed to form to determine whether field should be displayed and submitted. Related to #374

Closes #374

Closes #401

barmac · 2022-11-24T16:05:55Z

This is now ready for review. It implements the "hide if" logic and includes getSchemaVariables implementation for hide conditions.

pinussilvestrus

Looks good to me! Just one comment, otherwise, let's merge and bring this out of the door 👍

pinussilvestrus · 2022-11-25T13:23:19Z

packages/form-js-editor/src/features/properties-panel/entries/ConditionEntry.js

+
+  const variablesList = useVariables();
+
+  // keep stable reference until https://github.com/bpmn-io/properties-panel/issues/196 is fixed


That might be resolved now via 3a51c37

Indeed! Let's check this in action.

pinussilvestrus

great work!🥇

bpmn-io-tasks bot added the needs review Review pending label Nov 14, 2022

bpmn-io-tasks bot assigned barmac Nov 14, 2022

barmac requested review from pinussilvestrus, christian-konrad and Skaiir November 14, 2022 12:53

barmac force-pushed the 374-conditional-rendering branch from 4b0d4fc to a7a536a Compare November 14, 2022 13:47

barmac commented Nov 15, 2022

View reviewed changes

packages/form-js-viewer/src/Form.js Outdated Show resolved Hide resolved

barmac commented Nov 15, 2022

View reviewed changes

christian-konrad reviewed Nov 15, 2022

View reviewed changes

nikku reviewed Nov 16, 2022

View reviewed changes

pinussilvestrus reviewed Nov 17, 2022

View reviewed changes

barmac force-pushed the 374-conditional-rendering branch from daad00c to 8d73786 Compare November 17, 2022 13:21

barmac requested review from nikku and pinussilvestrus November 17, 2022 15:43

barmac mentioned this pull request Nov 17, 2022

Use "hideIf" logic for field condition #415

Closed

Skaiir reviewed Nov 18, 2022

View reviewed changes

pinussilvestrus mentioned this pull request Nov 21, 2022

Image view component with expression properties #385

Merged

barmac force-pushed the 374-conditional-rendering branch from 2e3ae68 to 2ee4183 Compare November 24, 2022 13:58

barmac mentioned this pull request Nov 24, 2022

feat: retrieve variables mentioned in conditions #423

Closed

barmac and others added 13 commits November 24, 2022 17:04

feat(viewer): add condition property to field

5451357

Condition is a FEEL unary test which can use any values passed to form to determine whether field should be displayed and submitted. Related to #374

chore: ignore luxon circular dependency

b6eaa94

feat(editor): add condition group to properties panel

c4205e7

Closes #374

chore: remove unnecessary change

4553e2a

fix(condition): removed reliance on intialData

3fbb320

feat(editor): extract useVariables

0d78530

docs: document useCondition

cb3f0f9

chore: add comment

75b45c8

feat: make condition checker report error on broken condition

7ee5473

test: add conditional field to main package test

d33c813

feat(viewer): use conditional#hide for conditions

ee54b1e

feat(editor): use conditional#hide

2646e81

feat: retrieve variables mentioned in conditions

894c28d

Closes #401

barmac force-pushed the 374-conditional-rendering branch from 4be7411 to 894c28d Compare November 24, 2022 16:05

barmac marked this pull request as ready for review November 24, 2022 16:05

bpmn-io-tasks bot added needs review Review pending and removed in progress Currently worked on labels Nov 24, 2022

chore: update field description

1e134cc

pinussilvestrus reviewed Nov 25, 2022

View reviewed changes

barmac requested a review from pinussilvestrus November 25, 2022 13:33

chore: do not memoize variables

edb09b9

barmac force-pushed the 374-conditional-rendering branch from 7725407 to edb09b9 Compare November 25, 2022 13:37

pinussilvestrus approved these changes Nov 25, 2022

View reviewed changes

barmac merged commit 7890fc8 into develop Nov 25, 2022

barmac deleted the 374-conditional-rendering branch November 25, 2022 14:01

bpmn-io-tasks bot removed the needs review Review pending label Nov 25, 2022

philippfromme mentioned this pull request Jan 5, 2023

Release camunda-modeler v5.7.0 camunda/camunda-modeler#3361

Closed

41 tasks


		// TODO(@barmac): remove once https://github.com/moment/luxon/issues/193 is resolved
		function onwarn(warning, warn) {

		});


		it('should use give precedence to form data to check condition', function() {


		const variablesList = useVariables();

		// keep stable reference until https://github.com/bpmn-io/properties-panel/issues/196 is fixed

Add conditional rendering #403

Add conditional rendering #403

Conversation

barmac commented Nov 14, 2022 • edited by pinussilvestrus Loading

barmac commented Nov 14, 2022

barmac commented Nov 14, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nikku Nov 16, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nikku left a comment

Choose a reason for hiding this comment

barmac commented Nov 16, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

barmac commented Nov 17, 2022 • edited Loading

Skaiir left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Skaiir Nov 16, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Skaiir commented Nov 18, 2022 • edited Loading

barmac commented Nov 18, 2022

christian-konrad commented Nov 23, 2022

barmac commented Nov 24, 2022

pinussilvestrus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pinussilvestrus left a comment • edited Loading

Choose a reason for hiding this comment

barmac commented Nov 14, 2022 •

edited by pinussilvestrus

Loading

nikku Nov 16, 2022 •

edited

Loading

barmac commented Nov 17, 2022 •

edited

Loading

Skaiir Nov 16, 2022 •

edited

Loading

Skaiir commented Nov 18, 2022 •

edited

Loading

pinussilvestrus left a comment •

edited

Loading