-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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 Raw JSON Form Field Input #9078
Changes from all commits
7c1ba1c
a17507e
a89cb76
90e7dc2
dbb67d4
c91f188
8bed07f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,85 @@ | ||
import { FormFieldInputContainer } from '@/object-record/record-field/form-types/components/FormFieldInputContainer'; | ||
import { FormFieldInputInputContainer } from '@/object-record/record-field/form-types/components/FormFieldInputInputContainer'; | ||
import { FormFieldInputRowContainer } from '@/object-record/record-field/form-types/components/FormFieldInputRowContainer'; | ||
import { TextVariableEditor } from '@/object-record/record-field/form-types/components/TextVariableEditor'; | ||
import { useTextVariableEditor } from '@/object-record/record-field/form-types/hooks/useTextVariableEditor'; | ||
import { VariablePickerComponent } from '@/object-record/record-field/form-types/types/VariablePickerComponent'; | ||
import { InputLabel } from '@/ui/input/components/InputLabel'; | ||
import { useId } from 'react'; | ||
import { isDefined } from 'twenty-ui'; | ||
import { turnIntoEmptyStringIfWhitespacesOnly } from '~/utils/string/turnIntoEmptyStringIfWhitespacesOnly'; | ||
|
||
type FormRawJsonFieldInputProps = { | ||
label?: string; | ||
defaultValue: string | null | undefined; | ||
placeholder: string; | ||
onPersist: (value: string | null) => void; | ||
readonly?: boolean; | ||
VariablePicker?: VariablePickerComponent; | ||
}; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. not for this PR, but I think we will build a base props interface for those components There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We could! It would be great to harmonize the props every component takes. For instance, only some fields take a readonly prop. We might want to add this behavior to every component. |
||
|
||
export const FormRawJsonFieldInput = ({ | ||
label, | ||
defaultValue, | ||
placeholder, | ||
onPersist, | ||
readonly, | ||
VariablePicker, | ||
}: FormRawJsonFieldInputProps) => { | ||
const inputId = useId(); | ||
|
||
const editor = useTextVariableEditor({ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. style: onUpdate callback is recreated on every render. Consider memoizing with useCallback to prevent unnecessary re-renders. |
||
placeholder, | ||
multiline: true, | ||
readonly, | ||
defaultValue: defaultValue ?? undefined, | ||
onUpdate: (editor) => { | ||
const text = turnIntoEmptyStringIfWhitespacesOnly(editor.getText()); | ||
|
||
if (text === '') { | ||
onPersist(null); | ||
|
||
return; | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am not a big fan of this. We will want to valid the field indeed. But we do not want to avoid persisting without letting the user notice. This is why we do not have any validation on links and email yet, we leave the user save whatever he wants. Next step would be to discuss with Thomas on a common behavior for field content validation. Also maybe not let the workflow being activated if a field is invalid so we know the workflow would fail. I would:
Wdyt? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree that we still need to talk about validation. I already added validation to the Date field. The input doesn't allow you to submit invalid dates. |
||
|
||
onPersist(text); | ||
}, | ||
}); | ||
|
||
const handleVariableTagInsert = (variableName: string) => { | ||
if (!isDefined(editor)) { | ||
throw new Error( | ||
'Expected the editor to be defined when a variable is selected', | ||
); | ||
} | ||
|
||
editor.commands.insertVariableTag(variableName); | ||
}; | ||
Comment on lines
+49
to
+57
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. logic: handleVariableTagInsert throws if editor undefined, but component renders null in that case. This error should never occur - consider removing the check or logging a warning instead. |
||
|
||
if (!isDefined(editor)) { | ||
return null; | ||
} | ||
|
||
return ( | ||
<FormFieldInputContainer> | ||
{label ? <InputLabel>{label}</InputLabel> : null} | ||
|
||
<FormFieldInputRowContainer multiline> | ||
<FormFieldInputInputContainer | ||
hasRightElement={isDefined(VariablePicker)} | ||
multiline | ||
> | ||
<TextVariableEditor editor={editor} multiline readonly={readonly} /> | ||
</FormFieldInputInputContainer> | ||
|
||
{VariablePicker ? ( | ||
<VariablePicker | ||
inputId={inputId} | ||
multiline | ||
onVariableSelect={handleVariableTagInsert} | ||
/> | ||
) : null} | ||
</FormFieldInputRowContainer> | ||
</FormFieldInputContainer> | ||
); | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,244 @@ | ||
import { expect } from '@storybook/jest'; | ||
import { Meta, StoryObj } from '@storybook/react'; | ||
import { fn, userEvent, waitFor, within } from '@storybook/test'; | ||
import { FormRawJsonFieldInput } from '../FormRawJsonFieldInput'; | ||
|
||
const meta: Meta<typeof FormRawJsonFieldInput> = { | ||
title: 'UI/Data/Field/Form/Input/FormRawJsonFieldInput', | ||
component: FormRawJsonFieldInput, | ||
args: {}, | ||
argTypes: {}, | ||
}; | ||
|
||
export default meta; | ||
|
||
type Story = StoryObj<typeof FormRawJsonFieldInput>; | ||
|
||
export const Default: Story = { | ||
args: { | ||
label: 'JSON field', | ||
placeholder: 'Enter valid json', | ||
}, | ||
play: async ({ canvasElement }) => { | ||
const canvas = within(canvasElement); | ||
|
||
await canvas.findByText('JSON field'); | ||
}, | ||
}; | ||
|
||
export const Readonly: Story = { | ||
args: { | ||
label: 'JSON field', | ||
placeholder: 'Enter valid json', | ||
readonly: true, | ||
onPersist: fn(), | ||
}, | ||
play: async ({ canvasElement, args }) => { | ||
const editor = canvasElement.querySelector('.ProseMirror > p'); | ||
expect(editor).toBeVisible(); | ||
|
||
await userEvent.type(editor, '{{ "a": {{ "b" : "d" } }'); | ||
|
||
await waitFor(() => { | ||
const allParagraphs = canvasElement.querySelectorAll('.ProseMirror > p'); | ||
expect(allParagraphs).toHaveLength(1); | ||
expect(allParagraphs[0]).toHaveTextContent(''); | ||
}); | ||
|
||
expect(args.onPersist).not.toHaveBeenCalled(); | ||
}, | ||
}; | ||
|
||
export const SaveValidJson: Story = { | ||
args: { | ||
placeholder: 'Enter valid json', | ||
onPersist: fn(), | ||
}, | ||
play: async ({ canvasElement, args }) => { | ||
const editor = canvasElement.querySelector('.ProseMirror > p'); | ||
expect(editor).toBeVisible(); | ||
|
||
await userEvent.type(editor, '{{ "a": {{ "b" : "d" } }'); | ||
|
||
await waitFor(() => { | ||
expect(args.onPersist).toHaveBeenCalledWith('{ "a": { "b" : "d" } }'); | ||
}); | ||
}, | ||
}; | ||
|
||
export const DoesNotIgnoreInvalidJson: Story = { | ||
args: { | ||
placeholder: 'Enter valid json', | ||
onPersist: fn(), | ||
}, | ||
play: async ({ canvasElement, args }) => { | ||
const editor = canvasElement.querySelector('.ProseMirror > p'); | ||
expect(editor).toBeVisible(); | ||
|
||
await userEvent.type(editor, 'lol'); | ||
|
||
await userEvent.click(canvasElement); | ||
|
||
expect(args.onPersist).toHaveBeenCalledWith('lol'); | ||
}, | ||
}; | ||
|
||
export const DisplayDefaultValueWithVariablesProperly: Story = { | ||
args: { | ||
placeholder: 'Enter valid json', | ||
defaultValue: '{ "a": { "b" : {{var.test}} } }', | ||
}, | ||
play: async ({ canvasElement }) => { | ||
const canvas = within(canvasElement); | ||
|
||
await canvas.findByText(/{ "a": { "b" : /); | ||
|
||
await waitFor(() => { | ||
const variableTag = canvasElement.querySelector( | ||
'[data-type="variableTag"]', | ||
); | ||
|
||
expect(variableTag).toBeVisible(); | ||
expect(variableTag).toHaveTextContent('test'); | ||
}); | ||
|
||
await canvas.findByText(/ } }/); | ||
}, | ||
}; | ||
|
||
export const InsertVariableInTheMiddleOnTextInput: Story = { | ||
args: { | ||
placeholder: 'Enter valid json', | ||
VariablePicker: ({ onVariableSelect }) => { | ||
return ( | ||
<button | ||
onClick={() => { | ||
onVariableSelect('{{test}}'); | ||
}} | ||
> | ||
Add variable | ||
</button> | ||
); | ||
}, | ||
}, | ||
play: async ({ canvasElement, args }) => { | ||
const canvas = within(canvasElement); | ||
|
||
const editor = canvasElement.querySelector('.ProseMirror > p'); | ||
expect(editor).toBeVisible(); | ||
|
||
const addVariableButton = await canvas.findByRole('button', { | ||
name: 'Add variable', | ||
}); | ||
|
||
await userEvent.type(editor, '{{ "a": {{ "b" : '); | ||
|
||
await userEvent.click(addVariableButton); | ||
|
||
await userEvent.type(editor, ' } }'); | ||
|
||
await Promise.all([ | ||
waitFor(() => { | ||
expect(args.onPersist).toHaveBeenCalledWith( | ||
'{ "a": { "b" : {{test}} } }', | ||
); | ||
}), | ||
]); | ||
}, | ||
}; | ||
|
||
export const CanUseVariableAsObjectProperty: Story = { | ||
args: { | ||
placeholder: 'Enter valid json', | ||
VariablePicker: ({ onVariableSelect }) => { | ||
return ( | ||
<button | ||
onClick={() => { | ||
onVariableSelect('{{test}}'); | ||
}} | ||
> | ||
Add variable | ||
</button> | ||
); | ||
}, | ||
}, | ||
play: async ({ canvasElement, args }) => { | ||
const canvas = within(canvasElement); | ||
|
||
const editor = canvasElement.querySelector('.ProseMirror > p'); | ||
expect(editor).toBeVisible(); | ||
|
||
const addVariableButton = await canvas.findByRole('button', { | ||
name: 'Add variable', | ||
}); | ||
|
||
await userEvent.type(editor, '{{ "'); | ||
|
||
await userEvent.click(addVariableButton); | ||
|
||
await userEvent.type(editor, '": 2 }'); | ||
|
||
await waitFor(() => { | ||
expect(args.onPersist).toHaveBeenCalledWith('{ "{{test}}": 2 }'); | ||
}); | ||
}, | ||
}; | ||
|
||
export const ClearField: Story = { | ||
args: { | ||
placeholder: 'Enter valid json', | ||
defaultValue: '{ "a": 2 }', | ||
}, | ||
play: async ({ canvasElement, args }) => { | ||
const defaultValueStringLength = args.defaultValue!.length; | ||
|
||
const editor = canvasElement.querySelector('.ProseMirror > p'); | ||
expect(editor).toBeVisible(); | ||
|
||
await Promise.all([ | ||
userEvent.type(editor, `{Backspace>${defaultValueStringLength}}`), | ||
|
||
waitFor(() => { | ||
expect(args.onPersist).toHaveBeenCalledWith(null); | ||
}), | ||
Comment on lines
+198
to
+203
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. logic: Using Promise.all with a single waitFor condition is unnecessary and could mask timing issues. Consider removing Promise.all wrapper. |
||
]); | ||
}, | ||
}; | ||
|
||
/** | ||
* Line breaks are not authorized in JSON strings. Users should instead put newlines characters themselves. | ||
* See https://stackoverflow.com/a/42073. | ||
*/ | ||
export const DoesNotBreakWhenUserInsertsNewlineInJsonString: Story = { | ||
args: { | ||
placeholder: 'Enter valid json', | ||
onPersist: fn(), | ||
}, | ||
play: async ({ canvasElement, args }) => { | ||
const editor = canvasElement.querySelector('.ProseMirror > p'); | ||
expect(editor).toBeVisible(); | ||
|
||
await userEvent.type(editor, '"a{Enter}b"'); | ||
|
||
await userEvent.click(canvasElement); | ||
|
||
expect(args.onPersist).toHaveBeenCalled(); | ||
}, | ||
}; | ||
|
||
export const AcceptsJsonEncodedNewline: Story = { | ||
args: { | ||
placeholder: 'Enter valid json', | ||
onPersist: fn(), | ||
}, | ||
play: async ({ canvasElement, args }) => { | ||
const editor = canvasElement.querySelector('.ProseMirror > p'); | ||
expect(editor).toBeVisible(); | ||
|
||
await userEvent.type(editor, '"a\\nb"'); | ||
|
||
await userEvent.click(canvasElement); | ||
|
||
expect(args.onPersist).toHaveBeenCalledWith('"a\\nb"'); | ||
}, | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logic: Type casting to string | undefined may be insufficient for raw JSON - consider using JsonValue type instead to properly handle nested objects and arrays