From 2322075120fdd3c4f62e831bd42b9a0ca8b9e115 Mon Sep 17 00:00:00 2001 From: Bogdan Chadkin Date: Mon, 13 May 2024 13:56:10 +0300 Subject: [PATCH 1/6] refactor: use native validation for parameter form Here switched to native html solution for validation in parameter variable form. Added three new form utilities to replicate existing behavior with closing errored form only on the second attempt when all errors are shown. - Form - useFormField - checkCanRequestSubmit --- .../settings-panel/variable-popover.tsx | 144 ++++++++++------- .../variables-section.stories.tsx | 14 ++ .../app/shared/form-utils/form-utils.ts | 61 ------- .../app/shared/form-utils/form-utils.tsx | 149 ++++++++++++++++++ 4 files changed, 251 insertions(+), 117 deletions(-) delete mode 100644 apps/builder/app/shared/form-utils/form-utils.ts create mode 100644 apps/builder/app/shared/form-utils/form-utils.tsx diff --git a/apps/builder/app/builder/features/settings-panel/variable-popover.tsx b/apps/builder/app/builder/features/settings-panel/variable-popover.tsx index 36d112fe18d..c0a3834085e 100644 --- a/apps/builder/app/builder/features/settings-panel/variable-popover.tsx +++ b/apps/builder/app/builder/features/settings-panel/variable-popover.tsx @@ -4,6 +4,7 @@ import { type ReactNode, type Ref, type RefObject, + type ForwardedRef, forwardRef, useId, useState, @@ -11,6 +12,7 @@ import { useRef, createContext, useContext, + useCallback, } from "react"; import { mergeRefs } from "@react-aria/utils"; import { CopyIcon, RefreshIcon } from "@webstudio-is/icons"; @@ -22,6 +24,7 @@ import { FloatingPanelPopoverContent, FloatingPanelPopoverTitle, FloatingPanelPopoverTrigger, + Grid, InputErrorsTooltip, InputField, Label, @@ -53,6 +56,9 @@ import { type Field, composeFields, type ComposedFields, + useFormField, + Form, + checkCanRequestSubmit, } from "~/shared/form-utils"; import { $userPlanFeatures } from "~/builder/shared/nano-states"; import { BindingPopoverProvider } from "~/builder/shared/binding-popover"; @@ -65,6 +71,56 @@ import { import { ResourceForm, SystemResourceForm } from "./resource-panel"; import { generateCurl } from "./curl"; +const NameField = ({ defaultValue }: { defaultValue: string }) => { + const nameId = useId(); + const { ref, error, props } = useFormField({ + defaultValue, + validate: useCallback( + (value: string) => (value.trim().length === 0 ? "Name is required" : ""), + [] + ), + }); + return ( + + + + + + + ); +}; + +const ParameterForm = forwardRef( + ({ variable }, ref) => { + return ( +
{ + const formData = new FormData(event.currentTarget); + const name = String(formData.get("name")); + // only existing parameter variables can be renamed + if (variable === undefined) { + return; + } + serverSyncStore.createTransaction([$dataSources], (dataSources) => { + dataSources.set(variable.id, { ...variable, name }); + }); + }} + > + + + ); + } +); +ParameterForm.displayName = "ParameterForm"; + /** * convert value expression to js value * validating out accessing any identifier @@ -115,28 +171,6 @@ type PanelApi = ComposedFields & { save: () => void; }; -const ParameterForm = forwardRef< - undefined | PanelApi, - { variable?: DataSource; nameField: Field } ->(({ variable, nameField }, ref) => { - const form = composeFields(nameField); - useImperativeHandle(ref, () => ({ - ...form, - save: () => { - // only existing parameter variables can be renamed - if (variable === undefined) { - return; - } - const name = nameField.value; - serverSyncStore.createTransaction([$dataSources], (dataSources) => { - dataSources.set(variable.id, { ...variable, name }); - }); - }, - })); - return <>; -}); -ParameterForm.displayName = "ParameterForm"; - const useValuePanelRef = ({ ref, variable, @@ -356,9 +390,11 @@ JsonForm.displayName = "JsonForm"; const VariablePanel = forwardRef< undefined | PanelApi, { + formRef: ForwardedRef; variable?: DataSource; + onSubmit: () => void; } ->(({ variable }, ref) => { +>(({ formRef, variable, onSubmit }, ref) => { const { allowDynamicData } = useStore($userPlanFeatures); const resources = useStore($resources); @@ -474,63 +510,58 @@ const VariablePanel = forwardRef< ); if (variableType === "parameter") { - return ( - <> - {nameFieldElement} - - - ); + return ; } if (variableType === "string") { return ( - <> +
{nameFieldElement} {typeFieldElement} - + ); } if (variableType === "number") { return ( - <> +
{nameFieldElement} {typeFieldElement} - + ); } if (variableType === "boolean") { return ( - <> +
{nameFieldElement} {typeFieldElement} - + ); } if (variableType === "json") { return ( - <> +
{nameFieldElement} {typeFieldElement} - + ); } if (variableType === "resource") { return ( - <> +
{nameFieldElement} {typeFieldElement} - + ); } if (variableType === "system-resource") { return ( - <> +
{nameFieldElement} {typeFieldElement} - + ); } @@ -563,8 +594,15 @@ export const VariablePopoverTrigger = forwardRef< const bindingPopoverContainerRef = useRef(null); const panelRef = useRef(); const resources = useStore($resources); + const form = useRef(null); const saveAndClose = () => { + if (form.current) { + if (checkCanRequestSubmit(form.current) === false) { + return; + } + form.current.requestSubmit(); + } if (panelRef.current) { if (panelRef.current.allErrorsVisible === false) { panelRef.current.showAllErrors(); @@ -615,22 +653,16 @@ export const VariablePopoverTrigger = forwardRef< pb: theme.spacing[9], }} > -
{ - event.preventDefault(); - saveAndClose(); - }} + - {/* submit is not triggered when press enter on input without submit button */} - - - - - + +
{/* put after content to avoid auto focusing "Close" button */} diff --git a/apps/builder/app/builder/features/settings-panel/variables-section.stories.tsx b/apps/builder/app/builder/features/settings-panel/variables-section.stories.tsx index ecbee4b99de..f2e9b072b31 100644 --- a/apps/builder/app/builder/features/settings-panel/variables-section.stories.tsx +++ b/apps/builder/app/builder/features/settings-panel/variables-section.stories.tsx @@ -6,6 +6,7 @@ import { $selectedInstanceSelector, $selectedPageId, $instances, + $dataSources, } from "~/shared/nano-states"; import { registerContainers } from "~/shared/sync"; import { createDefaultPages } from "@webstudio-is/project-build"; @@ -26,6 +27,19 @@ $selectedPageId.set("home"); $pages.set( createDefaultPages({ rootInstanceId: "root", systemDataSourceId: "system" }) ); +$dataSources.set( + new Map([ + [ + "systemId", + { + id: "systemId", + scopeInstanceId: "root", + name: "system", + type: "parameter", + }, + ], + ]) +); export const VariablesSection: StoryObj = { render: () => ( diff --git a/apps/builder/app/shared/form-utils/form-utils.ts b/apps/builder/app/shared/form-utils/form-utils.ts deleted file mode 100644 index 8c83d325eb3..00000000000 --- a/apps/builder/app/shared/form-utils/form-utils.ts +++ /dev/null @@ -1,61 +0,0 @@ -import { useState } from "react"; - -export type Field = { - value: Type; - error: undefined | string; - isErrorVisible: boolean; - valid: boolean; - onChange: (value: Type) => void; - onBlur: () => void; -}; - -export const useField = ({ - initialValue, - validate, -}: { - initialValue: Type; - validate: (value: Type) => undefined | string; -}): Field => { - const [value, setValue] = useState(initialValue); - const [touched, setTouched] = useState(false); - const error = validate?.(value); - return { - value, - // show error only when user stop interactinig with control - error: touched ? error : undefined, - // inform when user see the error and not need to trigger it - isErrorVisible: error === undefined || touched, - valid: error === undefined, - // hide error when user is typing - onChange: (value) => { - setValue(value); - setTouched(false); - }, - // show error when user focus on another control - onBlur: () => { - setTouched(true); - }, - }; -}; - -export type ComposedFields = { - valid: boolean; - allErrorsVisible: boolean; - showAllErrors: () => void; -}; - -export const composeFields = ( - // allow passing fields with more tight types - // by ensuring their onChange will not refine - ...fields: Omit, "onChange">[] -): ComposedFields => { - return { - valid: fields.every((field) => field.valid), - allErrorsVisible: fields.every((field) => field.isErrorVisible), - showAllErrors: () => { - for (const field of fields) { - field.onBlur(); - } - }, - }; -}; diff --git a/apps/builder/app/shared/form-utils/form-utils.tsx b/apps/builder/app/shared/form-utils/form-utils.tsx new file mode 100644 index 00000000000..8cd01b17fdc --- /dev/null +++ b/apps/builder/app/shared/form-utils/form-utils.tsx @@ -0,0 +1,149 @@ +import type { + ChangeEvent, + FocusEvent, + FormEvent, + InvalidEvent, + ReactNode, +} from "react"; +import { forwardRef, useEffect, useRef, useState } from "react"; + +export type Field = { + value: Type; + error: undefined | string; + isErrorVisible: boolean; + valid: boolean; + onChange: (value: Type) => void; + onBlur: () => void; +}; + +export const useField = ({ + initialValue, + validate, +}: { + initialValue: Type; + validate: (value: Type) => undefined | string; +}): Field => { + const [value, setValue] = useState(initialValue); + const [touched, setTouched] = useState(false); + const error = validate?.(value); + return { + value, + // show error only when user stop interactinig with control + error: touched ? error : undefined, + // inform when user see the error and not need to trigger it + isErrorVisible: error === undefined || touched, + valid: error === undefined, + // hide error when user is typing + onChange: (value) => { + setValue(value); + setTouched(false); + }, + // show error when user focus on another control + onBlur: () => { + setTouched(true); + }, + }; +}; + +export type ComposedFields = { + valid: boolean; + allErrorsVisible: boolean; + showAllErrors: () => void; +}; + +export const composeFields = ( + // allow passing fields with more tight types + // by ensuring their onChange will not refine + ...fields: Omit, "onChange">[] +): ComposedFields => { + return { + valid: fields.every((field) => field.valid), + allErrorsVisible: fields.every((field) => field.isErrorVisible), + showAllErrors: () => { + for (const field of fields) { + field.onBlur(); + } + }, + }; +}; + +export const useFormField = ({ + defaultValue, + validate, +}: { + defaultValue: string; + validate: (value: string) => string; +}) => { + const [error, setError] = useState(""); + const ref = useRef(null); + // validate initial value + useEffect(() => { + ref.current?.setCustomValidity(validate(defaultValue)); + }, [defaultValue, validate]); + const props = { + onChange: (event: ChangeEvent) => { + setError(""); + event.target.setCustomValidity(validate(event.target.value)); + }, + onBlur: (event: FocusEvent) => { + event.target.checkValidity(); + }, + onInvalid: (event: InvalidEvent) => { + setError(event.target.validationMessage); + }, + }; + return { + ref, + error, + props, + }; +}; + +/** + * prevents default navigation + * supports submit on enter + * avoids submitting when invalid + * resets attempts + * does not affect layout + */ +export const Form = forwardRef< + HTMLFormElement, + { onSubmit: (event: FormEvent) => void; children: ReactNode } +>(({ onSubmit, children }, ref) => { + return ( +
{ + event.preventDefault(); + if (event.currentTarget.checkValidity() === false) { + return; + } + onSubmit(event); + }} + onChange={(event) => { + delete event.currentTarget.dataset.attemptedSubmit; + }} + > + {/* submit is not triggered when press enter on input without submit button */} + + {children} +
+ ); +}); + +/** + * checks validity and allow request a submit on second attempt to not block user + * attempts are reset when change event is propagated + */ +export const checkCanRequestSubmit = (form: HTMLFormElement) => { + if ( + form.checkValidity() === false && + form.dataset.attemptedSubmit === undefined + ) { + form.dataset.attemptedSubmit = "true"; + return false; + } + return true; +}; From bfdd715b1adf3efe5a2640d5f3a69ebb3795324e Mon Sep 17 00:00:00 2001 From: Bogdan Chadkin Date: Mon, 13 May 2024 14:17:55 +0300 Subject: [PATCH 2/6] Deprecated useField --- apps/builder/app/shared/form-utils/form-utils.tsx | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/apps/builder/app/shared/form-utils/form-utils.tsx b/apps/builder/app/shared/form-utils/form-utils.tsx index 8cd01b17fdc..fa74cf9dabb 100644 --- a/apps/builder/app/shared/form-utils/form-utils.tsx +++ b/apps/builder/app/shared/form-utils/form-utils.tsx @@ -16,6 +16,9 @@ export type Field = { onBlur: () => void; }; +/** + * @deprecated switch to useFormField or any other native validation + */ export const useField = ({ initialValue, validate, @@ -51,6 +54,9 @@ export type ComposedFields = { showAllErrors: () => void; }; +/** + * @deprecated switch to useFormField or any other native validation + */ export const composeFields = ( // allow passing fields with more tight types // by ensuring their onChange will not refine From d0af634c52880804325c077f6f3dabc2733c0581 Mon Sep 17 00:00:00 2001 From: Bogdan Chadkin Date: Mon, 13 May 2024 14:37:25 +0300 Subject: [PATCH 3/6] Autosave on blur --- .../features/settings-panel/variable-popover.tsx | 15 +++++++++++++-- apps/builder/app/shared/form-utils/form-utils.tsx | 2 ++ 2 files changed, 15 insertions(+), 2 deletions(-) diff --git a/apps/builder/app/builder/features/settings-panel/variable-popover.tsx b/apps/builder/app/builder/features/settings-panel/variable-popover.tsx index c0a3834085e..11e0b8b70d0 100644 --- a/apps/builder/app/builder/features/settings-panel/variable-popover.tsx +++ b/apps/builder/app/builder/features/settings-panel/variable-popover.tsx @@ -13,6 +13,7 @@ import { createContext, useContext, useCallback, + type FocusEvent, } from "react"; import { mergeRefs } from "@react-aria/utils"; import { CopyIcon, RefreshIcon } from "@webstudio-is/icons"; @@ -71,7 +72,13 @@ import { import { ResourceForm, SystemResourceForm } from "./resource-panel"; import { generateCurl } from "./curl"; -const NameField = ({ defaultValue }: { defaultValue: string }) => { +const NameField = ({ + defaultValue, + onBlur, +}: { + defaultValue: string; + onBlur?: (event: FocusEvent) => void; +}) => { const nameId = useId(); const { ref, error, props } = useFormField({ defaultValue, @@ -91,6 +98,7 @@ const NameField = ({ defaultValue }: { defaultValue: string }) => { color={error ? "error" : undefined} defaultValue={defaultValue} {...props} + onBlur={onBlur ?? props.onBlur} /> @@ -114,7 +122,10 @@ const ParameterForm = forwardRef( }); }} > - + event.target.form?.requestSubmit()} + /> ); } diff --git a/apps/builder/app/shared/form-utils/form-utils.tsx b/apps/builder/app/shared/form-utils/form-utils.tsx index fa74cf9dabb..749a44df8e4 100644 --- a/apps/builder/app/shared/form-utils/form-utils.tsx +++ b/apps/builder/app/shared/form-utils/form-utils.tsx @@ -110,6 +110,7 @@ export const useFormField = ({ * supports submit on enter * avoids submitting when invalid * resets attempts + * disables native tooltips * does not affect layout */ export const Form = forwardRef< @@ -121,6 +122,7 @@ export const Form = forwardRef< ref={ref} // exclude from the flow style={{ display: "contents" }} + noValidate={true} onSubmit={(event) => { event.preventDefault(); if (event.currentTarget.checkValidity() === false) { From 9bd67ea242de1fbe04990d728a7ee9aad2f19d30 Mon Sep 17 00:00:00 2001 From: Bogdan Chadkin Date: Mon, 13 May 2024 16:15:09 +0300 Subject: [PATCH 4/6] Support escape out of the box --- .../features/settings-panel/variable-popover.tsx | 1 + apps/builder/app/shared/form-utils/form-utils.tsx | 13 +++++++++++++ 2 files changed, 14 insertions(+) diff --git a/apps/builder/app/builder/features/settings-panel/variable-popover.tsx b/apps/builder/app/builder/features/settings-panel/variable-popover.tsx index 11e0b8b70d0..f8f69ea589c 100644 --- a/apps/builder/app/builder/features/settings-panel/variable-popover.tsx +++ b/apps/builder/app/builder/features/settings-panel/variable-popover.tsx @@ -14,6 +14,7 @@ import { useContext, useCallback, type FocusEvent, + type KeyboardEvent, } from "react"; import { mergeRefs } from "@react-aria/utils"; import { CopyIcon, RefreshIcon } from "@webstudio-is/icons"; diff --git a/apps/builder/app/shared/form-utils/form-utils.tsx b/apps/builder/app/shared/form-utils/form-utils.tsx index 749a44df8e4..39d03ad8dd7 100644 --- a/apps/builder/app/shared/form-utils/form-utils.tsx +++ b/apps/builder/app/shared/form-utils/form-utils.tsx @@ -3,6 +3,7 @@ import type { FocusEvent, FormEvent, InvalidEvent, + KeyboardEvent, ReactNode, } from "react"; import { forwardRef, useEffect, useRef, useState } from "react"; @@ -97,6 +98,18 @@ export const useFormField = ({ onInvalid: (event: InvalidEvent) => { setError(event.target.validationMessage); }, + onKeyDown: (event: KeyboardEvent) => { + const input = event.currentTarget; + // reset on escape when default value is different + if (event.key === "Escape" && input.value !== input.defaultValue) { + // prevent propagating escape to dialog or popover + event.stopPropagation(); + input.value = input.defaultValue; + // revalidate after reset + setError(""); + input.setCustomValidity(validate(input.value)); + } + }, }; return { ref, From 165dc48827bae199a033f7e9438db7a9fd30e153 Mon Sep 17 00:00:00 2001 From: Bogdan Chadkin Date: Mon, 13 May 2024 16:20:03 +0300 Subject: [PATCH 5/6] Fix lint --- .../app/builder/features/settings-panel/variable-popover.tsx | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/apps/builder/app/builder/features/settings-panel/variable-popover.tsx b/apps/builder/app/builder/features/settings-panel/variable-popover.tsx index f8f69ea589c..cb65b906dff 100644 --- a/apps/builder/app/builder/features/settings-panel/variable-popover.tsx +++ b/apps/builder/app/builder/features/settings-panel/variable-popover.tsx @@ -5,6 +5,7 @@ import { type Ref, type RefObject, type ForwardedRef, + type FocusEvent, forwardRef, useId, useState, @@ -13,8 +14,6 @@ import { createContext, useContext, useCallback, - type FocusEvent, - type KeyboardEvent, } from "react"; import { mergeRefs } from "@react-aria/utils"; import { CopyIcon, RefreshIcon } from "@webstudio-is/icons"; From 36c1da15ac3aa3dff4276cdac30574a3394917a6 Mon Sep 17 00:00:00 2001 From: Bogdan Chadkin Date: Mon, 13 May 2024 17:23:31 +0300 Subject: [PATCH 6/6] simplify formatting --- .../settings-panel/variable-popover.tsx | 51 ++++++++++--------- 1 file changed, 27 insertions(+), 24 deletions(-) diff --git a/apps/builder/app/builder/features/settings-panel/variable-popover.tsx b/apps/builder/app/builder/features/settings-panel/variable-popover.tsx index cb65b906dff..e7aa782d53b 100644 --- a/apps/builder/app/builder/features/settings-panel/variable-popover.tsx +++ b/apps/builder/app/builder/features/settings-panel/variable-popover.tsx @@ -105,31 +105,34 @@ const NameField = ({ ); }; -const ParameterForm = forwardRef( - ({ variable }, ref) => { - return ( -
{ - const formData = new FormData(event.currentTarget); - const name = String(formData.get("name")); - // only existing parameter variables can be renamed - if (variable === undefined) { - return; - } - serverSyncStore.createTransaction([$dataSources], (dataSources) => { - dataSources.set(variable.id, { ...variable, name }); - }); - }} - > - event.target.form?.requestSubmit()} - /> - - ); +const ParameterForm = forwardRef< + HTMLFormElement, + { + variable?: DataSource; } -); +>(({ variable }, ref) => { + return ( +
{ + const formData = new FormData(event.currentTarget); + const name = String(formData.get("name")); + // only existing parameter variables can be renamed + if (variable === undefined) { + return; + } + serverSyncStore.createTransaction([$dataSources], (dataSources) => { + dataSources.set(variable.id, { ...variable, name }); + }); + }} + > + event.target.form?.requestSubmit()} + /> + + ); +}); ParameterForm.displayName = "ParameterForm"; /**