Create form field number #8634

Devessier · 2024-11-21T09:52:30Z

Refactor VariableTagInput to have a reusable low-level TipTap editor
Create three primitive form fields:
- Text
- Number
- Boolean

Notes

We should automatically recognize the placeholder to use for every FormFieldInput, as it's done for FieldInputs.

Design decisions

Our main challenge was for variables and inputs to be able to communicate between each other. We chose an API that adds some duplication but remains simple and doesn't rely on "hacks" to work. Common styles are centralized.

Demo

"Workflow" mode with variables:

FormFieldInput mode, without variables:

Behavior difference between fields that can contain variables and static content, and inputs that can have either a variable value or a static value:

...-front/src/modules/object-record/record-field/form-types/components/FormNumberFieldInput.tsx

packages/twenty-front/src/modules/workflow/components/WorkflowFormTextFieldInput.tsx

martmull · 2024-11-26T10:05:23Z

Functional testing observations

Boolean, we should be able to set no value at all, currently we are forced to set true or false (we might update the <BooleanFieldInput /> component) @Bonapara what do you think?
Number, we should not be able to type "string" values in input, only numbers
Number, I think we should not allow float values either
Number and Boolean, we should not be able to set string variable in boolean input, only boolean variables

thomtrp · 2024-11-26T10:21:52Z

Boolean, we should be able to set no value at all, currently we are forced to set true or false (we might update the <BooleanFieldInput /> component) @Bonapara what do you think?

@martmull this is a discussion we got 👍 This is the current behavior of our Boolean field, this is why we kept the same. It should not be seen as a primitive boolean field but more as a True/False field. If the user needs a 3 options field (like null, false, true) he can still use the select field for this.

...-front/src/modules/object-record/record-field/form-types/components/FormNumberFieldInput.tsx

...wenty-front/src/modules/object-record/record-field/form-types/hooks/useTextVariableEditor.ts

packages/twenty-front/src/modules/workflow/components/WorkflowFormFieldInputBase.tsx

packages/twenty-front/src/modules/workflow/components/WorkflowFormTextFieldInput.tsx

...es/twenty-front/src/modules/workflow/search-variables/components/SearchVariablesDropdown.tsx

packages/twenty-front/src/modules/workflow/search-variables/components/VariableTagInput.tsx

martmull

Hey nice job, some comments

...front/src/modules/object-record/record-field/form-types/components/FormBooleanFieldInput.tsx

martmull · 2024-11-26T10:11:11Z

...front/src/modules/object-record/record-field/form-types/components/FormBooleanFieldInput.tsx

+      Input={
+        <StyledBooleanInputContainer>
+          <BooleanInput
+            value={draftValue as boolean}


we should remove as boolean and use a format to boolean utils

It's an important subject, and we should discuss it first. There are two policies: either we cast values to the type we expect or discard values that don't match their expected type. By casting, I mean doing things like this: String(mightBeAStringOrSomethingElse).

I think we should be sure that draftValue is a boolean

martmull · 2024-11-26T10:17:25Z

...ty-front/src/modules/object-record/record-field/form-types/components/FormTextFieldInput.tsx

+  defaultValue,
+  placeholder,
+  onPersist,
+  multiline,


looks like multiline is not used. If not used we should remove it and related code

I'm not sure to understand why multiline wouldn't be used. It appears to be used multiple times in the commend for me. Am I missing something?

By saying not used, i mean i don't see any component that set this multiline parameter to true:

<ComponentUsingMultilineTrue ... multiline ... />

Did I miss one of them?

packages/twenty-front/src/modules/workflow/components/WorkflowFormFieldInputBase.tsx

martmull · 2024-11-26T10:22:13Z

packages/twenty-front/src/modules/workflow/components/WorkflowFormFieldInputBase.tsx

+        ) : (
+          <StyledVariableContainer>
+            <SortOrFilterChip
+              labelValue={extractVariableLabel(draftValue as string)}


we should remove as string

martmull · 2024-11-26T10:24:43Z

packages/twenty-front/src/modules/workflow/components/WorkflowFormNumberFieldInput.tsx

+  };
+
+  const handleChange = (newText: string) => {
+    setDraftValue(newText);


I think

if (!canBeCastAsNumberOrNull(newValue)) { return; }

should be used here!

we should also check for decimal and return if there is any

I duplicated the logic of the NumberFieldInput file; I'm unsure if we want to behave differently than this component.

martmull · 2024-11-26T10:28:00Z

packages/twenty-front/src/modules/workflow/components/WorkflowFormBooleanFieldInput.tsx

+        onPersist(false);
+      }}
+      onVariableTagInsert={(variable) => {
+        setDraftValue(variable);


we should check that variable is a boolean or not, and only set if boolean. That should be possible as variable type is provided in outputSchema

We can improve this in another PR. However, it will require many changes, which we must consider beforehand.

Created another issue to track that: #8787.

martmull · 2024-11-26T10:31:53Z

packages/twenty-front/src/modules/workflow/components/WorkflowFormNumberFieldInput.tsx

+        onPersist(null);
+      }}
+      onVariableTagInsert={(variable) => {
+        setDraftValue(variable);


we should check that variable has a type number, the best would be to remove the option in variable dropdown

As mentioned here, I think we can check it later: #8634 (comment).

packages/twenty-front/src/modules/workflow/search-variables/utils/initializeEditorContent.ts

martmull · 2024-11-26T10:36:30Z

packages/twenty-front/src/modules/workflow/search-variables/utils/extractVariableLabel.ts

+
+export const extractVariableLabel = (rawVariable: string) => {
+  const variableWithoutBrackets = rawVariable.replace(
+    /\{\{([^{}]+)\}\}/g,


can we use REGEX_VARIABLE_TAG here? I don't get the functional difference between the 2 regex

This regex captures what's inside brackets. The other one also captures brackets. We could refactor it to use a single regex.

ok nice, lets refactor then

I tried to reduce the number of regexes created to deal with variables, but every regex has a specific behavior, and I don't think it's excellent to unify them. We could make a one-size-fits-all regex, but it will necessarily be more complex and less readable, as well as the code using the regex. I think it's better to keep specialized regexes.

thomtrp

@Devessier I am not sure to get the full picture here. I was expecting an architecture like:

Primitive input (Boolean, Number, Text) that would be a common base for Form Field Input and Field Input
Primitive form field input (BooleanFormFieldInput, NumberFormFieldInput...)
Global component FormFieldInput to switch on it
WorkflowFormFieldInput that send the update functions, the variable component as right element and call the FormFieldInput as main input component.
Hard to implement for tiptap, so we could have a special WorkflowFormFieldInputWithTipTap (not the real component name)

In your code:

I do not see FormFieldInput actually used? To me it was supposed to be the base for workflow forms but also the futur forms without variables we will have to implement
It seems that we will have to add a WorkflowFieldInput for each type. But except for text using tiptap, isn't the logic to store value and use variables always the same?

Happy to discuss the architecture, maybe this is unrealistic. I would also like your opinion @lucasbordeau @charlesBochet

Devessier · 2024-11-26T11:15:10Z

@Devessier I am not sure to get the full picture here. I was expecting an architecture like:

Primitive input (Boolean, Number, Text) that would be a common base for Form Field Input and Field Input

Primitive form field input (BooleanFormFieldInput, NumberFormFieldInput...)

Global component FormFieldInput to switch on it

WorkflowFormFieldInput that send the update functions, the variable component as right element and call the FormFieldInput as main input component.

Hard to implement for tiptap, so we could have a special WorkflowFormFieldInputWithTipTap (not the real component name)

In your code:

I do not see FormFieldInput actually used? To me it was supposed to be the base for workflow forms but also the futur forms without variables we will have to implement

It seems that we will have to add a WorkflowFieldInput for each type. But except for text using tiptap, isn't the logic to store value and use variables always the same?

Happy to discuss the architecture, maybe this is unrealistic. I would also like your opinion @lucasbordeau

I'm unsure how we could have a standalone FormFieldInput component that could also be enhanced in WorkflowFormFieldInput. I might miss something, but I am happy to consider doing things differently.

Let's take WorkflowFormNumberFieldInput as an example. When we want to unlink a variable, we must call setDraftValue. However, if the WorkflowFormNumberFieldInput component was relying on FormNumberFieldInput to work, and provided that FormNumberFieldInput would need to own draftValue and setDraftValue to work as a standalone component, the parent component would need a way to set the state of its child component.
We could solve this by putting the input's state on Recoil, but it only solves some problems.

The FormNumberFieldInput component renders the FormFieldInputBase component, which gives it a standalone component look. The WorkflowFormNumberFieldInput component does so with WorkflowFormFieldInputBase.

We could find solutions to reuse standalone components and enhance them with variables, but I wonder if this is the simplest way.
Instead, I suggest reusing base components and common logic. For instance, both the WorkflowFormNumberFieldInput and FormNumberFieldInput components rely on the TextInput component.
We could also extract the common logic in a hook. For instance, draftValue, persistNumber and handleChange are common.
I didn't implement that in my PR, but I think it would be the way to go once we understood the common logic for each component.

I prefer some duplication of templating code (like creating more components) to trying to make things work together and finding in two weeks that one component shouldn't work the same way in workflows as it does in usual forms. Furthermore, we don't yet have a use case for FormFieldInput.

martmull · 2024-11-26T13:15:11Z

...-front/src/modules/object-record/record-field/form-types/components/FormNumberFieldInput.tsx

+      Input={
+        <StyledInput
+          placeholder={placeholder}
+          value={String(draftValue)}


To remain consistent with the codebase

Suggested change

value={String(draftValue)}

value={`${draftValue}`}

martmull · 2024-11-26T13:15:32Z

packages/twenty-front/src/modules/workflow/components/WorkflowFormNumberFieldInput.tsx

+        <StyledInput
+          inputId={inputId}
+          placeholder={placeholder}
+          value={String(draftValue)}


same value={${...}}

thomtrp

Amazing work @Devessier!! 💪 🔥

Let's wait for @lucasbordeau review but LGTM!

...-front/src/modules/object-record/record-field/form-types/components/SelectedVariableChip.tsx

thomtrp · 2024-11-28T12:39:51Z

packages/twenty-front/src/modules/workflow/components/WorkflowEditActionFormRecordCreate.tsx

+        const currentValue = formData[field.name] as JsonValue;
+
+        return (
+          <FormFieldInput


lucasbordeau · 2024-11-28T13:34:32Z

...ty-front/src/modules/object-record/record-field/form-types/components/FormTextFieldInput.tsx

+    },
+  });
+
+  const handleVariableTagInsert = (variable: string) => {


Could we avoid using just "variable" alone, either variableValue, variableContent or variableName ? As a first-time reader I don't clearly understand, which for this "meta" part of the code is important.

Yes 👌 Done.

...nty-front/src/modules/object-record/record-field/form-types/types/VariablePickerComponent.ts

...es/twenty-front/src/modules/workflow/search-variables/components/SearchVariablesDropdown.tsx

…ined

…riable string

…ble to take a variable picker

…rove type safety

github-actions · 2024-11-28T17:04:19Z

Thanks @Devessier for your contribution!
This marks your 30th PR on the repo. You're top 1% of all our contributors 🎉
See contributor page - Share on LinkedIn - Share on Twitter

thomtrp reviewed Nov 21, 2024

View reviewed changes

charlesBochet assigned lucasbordeau Nov 21, 2024

charlesBochet added the -PR: awaiting author label Nov 21, 2024

charlesBochet assigned Devessier Nov 21, 2024

Devessier force-pushed the create-form-field-number branch from 5c9f0fc to ca5fbe5 Compare November 25, 2024 10:00

Devessier commented Nov 25, 2024

View reviewed changes

packages/twenty-front/src/modules/workflow/components/WorkflowFormTextFieldInput.tsx Outdated Show resolved Hide resolved

Devessier marked this pull request as ready for review November 26, 2024 09:49

Devessier added -PR: awaiting review and removed -PR: awaiting author labels Nov 26, 2024

lucasbordeau reviewed Nov 26, 2024

View reviewed changes

martmull reviewed Nov 26, 2024

View reviewed changes

thomtrp reviewed Nov 26, 2024

View reviewed changes

martmull reviewed Nov 26, 2024

View reviewed changes

This was referenced Nov 28, 2024

Use hotkey scopes with Tiptap #8786

Open

Add type safety to variables in the frontend #8787

Open

thomtrp approved these changes Nov 28, 2024

View reviewed changes

lucasbordeau reviewed Nov 28, 2024

View reviewed changes

Devessier force-pushed the create-form-field-number branch from 786c826 to 4e5dc98 Compare November 28, 2024 14:17

Devessier added 8 commits November 28, 2024 17:47

refactor: decouple SearchVariablesDropdown from tiptap

6d91258

feat: start creating the form number field input

6262c33

refactor: extract the function that returns variable's label

c0554e9

feat: ensure the extractVariableLabel function always returns a string

f42ae84

refactor: always call the function that should always be properly def…

d8f8db1

…ined

refactor: use util function to extract variable label

729a60f

refactor: use a chip component to render the selected variable

80dc052

refactor: make all event listeners optional

d856c62

Devessier added 25 commits November 28, 2024 17:47

refactor: rename component

77650b0

refactor: rename component

dde06ad

feat: wip

55f1939

refactor: create a reusable component for input labels

5137b8c

feat: add a label and a placeholder to every field

f591351

refactor: reuse existing components for VariableTagInput

6f78981

refactor: un-export components and rename them

398d1fd

refactor: un-export style variable

7026a5f

refactor: wire label tag to text field input

45ce817

test: write stories for Form*FieldInput components

9b6508f

refactor: rename component property

44cfb7c

refactor: declare event handlers instead of inlining them

49d0882

refactor: get rid of abstractions and inline in workflow form fields

6386609

refactor: let the component take a component definition and render it

4ea3fb9

refactor: extract how we determine whether a value is a standalone va…

3f9e784

…riable string

refactor: remove workflow specific form fields and make form fields a…

69c2af0

…ble to take a variable picker

refactor: remove variable tag input

f39b005

refactor: delete workflow form field input base file

a99c072

refactor: delete FormFieldInputBase file

2628a69

refactor: simplify and rename variables

d36f996

refactor: rely on a single state to prevent impossible states and imp…

9342b6b

…rove type safety

fix: rename enum values to match the values used in the frontend

6837449

refactor: rename component to VariableChip

e1d33ae

test: add tests for the isStandaloneVariableString function

bddd14e

refactor: rename unclear variable parameter

fe39cfa

Devessier force-pushed the create-form-field-number branch from 4e5dc98 to fe39cfa Compare November 28, 2024 16:50

Devessier enabled auto-merge (squash) November 28, 2024 16:51

Devessier merged commit d73dc1a into main Nov 28, 2024
19 checks passed

Devessier deleted the create-form-field-number branch November 28, 2024 17:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create form field number #8634

Create form field number #8634

Devessier commented Nov 21, 2024 •

edited

Loading

martmull commented Nov 26, 2024 •

edited

Loading

thomtrp commented Nov 26, 2024

martmull left a comment

martmull Nov 26, 2024

Devessier Nov 26, 2024

martmull Nov 26, 2024

martmull Nov 26, 2024

Devessier Nov 26, 2024

martmull Nov 26, 2024

martmull Nov 26, 2024

martmull Nov 26, 2024

martmull Nov 26, 2024

Devessier Nov 28, 2024

martmull Nov 26, 2024

Devessier Nov 28, 2024

Devessier Nov 28, 2024

martmull Nov 26, 2024

Devessier Nov 28, 2024

martmull Nov 26, 2024

Devessier Nov 26, 2024

martmull Nov 26, 2024

Devessier Nov 28, 2024

thomtrp left a comment •

edited

Loading

Devessier commented Nov 26, 2024

martmull Nov 26, 2024

martmull Nov 26, 2024

thomtrp left a comment

thomtrp Nov 28, 2024

lucasbordeau Nov 28, 2024

Devessier Nov 28, 2024

github-actions bot commented Nov 28, 2024

Create form field number #8634

Create form field number #8634

Conversation

Devessier commented Nov 21, 2024 • edited Loading

Notes

Design decisions

Demo

martmull commented Nov 26, 2024 • edited Loading

thomtrp commented Nov 26, 2024

martmull 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thomtrp left a comment • edited Loading

Choose a reason for hiding this comment

Devessier commented Nov 26, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thomtrp 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

github-actions bot commented Nov 28, 2024

Devessier commented Nov 21, 2024 •

edited

Loading

martmull commented Nov 26, 2024 •

edited

Loading

thomtrp left a comment •

edited

Loading