From 7abc67c905a58d15a7d51318b8f02aa0773215aa Mon Sep 17 00:00:00 2001 From: Antoine Moreaux Date: Tue, 25 Feb 2025 11:37:36 +0100 Subject: [PATCH] refactor(forms): simplify form handling and button behavior (#10441) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Removed redundant handleSave and handleSubmit props in domain settings. Integrated form submission logic directly into form components, ensuring consistent behavior and reducing complexity. Updated button components to explicitly support the "type" attribute for improved accessibility and functionality. --------- Co-authored-by: Félix Malfait --- .../SaveAndCancelButtons/SaveButton.tsx | 1 + .../settings/components/SettingsRadioCard.tsx | 10 +- .../components/SettingsRadioCardContainer.tsx | 6 +- .../components/SSO/SettingsSSOOIDCForm.tsx | 3 + .../components/SSO/SettingsSSOSAMLForm.tsx | 7 +- .../SettingsSecurityApprovedAccessDomain.tsx | 146 +++++++++--------- .../SettingsSecuritySSOIdentifyProvider.tsx | 57 +++---- .../workspace/SettingsCustomDomain.tsx | 13 +- .../workspace/SettingsCustomDomainRecords.tsx | 9 +- .../settings/workspace/SettingsDomain.tsx | 87 ++++++----- .../settings/workspace/SettingsSubdomain.tsx | 13 +- .../src/input/button/components/Button.tsx | 10 +- .../input/button/components/LightButton.tsx | 3 + .../twenty-ui/src/input/components/Radio.tsx | 1 + 14 files changed, 199 insertions(+), 167 deletions(-) diff --git a/packages/twenty-front/src/modules/settings/components/SaveAndCancelButtons/SaveButton.tsx b/packages/twenty-front/src/modules/settings/components/SaveAndCancelButtons/SaveButton.tsx index 0df2edf0e..2ca74d0e6 100644 --- a/packages/twenty-front/src/modules/settings/components/SaveAndCancelButtons/SaveButton.tsx +++ b/packages/twenty-front/src/modules/settings/components/SaveAndCancelButtons/SaveButton.tsx @@ -15,6 +15,7 @@ export const SaveButton = ({ onSave, disabled }: SaveButtonProps) => { accent="blue" disabled={disabled} onClick={onSave} + type="submit" Icon={IconDeviceFloppy} /> ); diff --git a/packages/twenty-front/src/modules/settings/components/SettingsRadioCard.tsx b/packages/twenty-front/src/modules/settings/components/SettingsRadioCard.tsx index ed75acfdb..dede4159d 100644 --- a/packages/twenty-front/src/modules/settings/components/SettingsRadioCard.tsx +++ b/packages/twenty-front/src/modules/settings/components/SettingsRadioCard.tsx @@ -34,16 +34,18 @@ const StyledDescription = styled.div` type SettingsRadioCardProps = { value: string; - handleClick: (value: string) => void; + handleSelect: (value: string) => void; isSelected: boolean; title: string; description?: string; Icon?: IconComponent; + role?: string; + ariaChecked?: boolean; }; export const SettingsRadioCard = ({ value, - handleClick, + handleSelect, title, description, isSelected, @@ -51,8 +53,10 @@ export const SettingsRadioCard = ({ }: SettingsRadioCardProps) => { const theme = useTheme(); + const onClick = () => handleSelect(value); + return ( - handleClick(value)}> + {Icon && } {title && {title}} diff --git a/packages/twenty-front/src/modules/settings/components/SettingsRadioCardContainer.tsx b/packages/twenty-front/src/modules/settings/components/SettingsRadioCardContainer.tsx index 3b8f49941..4bdd2345f 100644 --- a/packages/twenty-front/src/modules/settings/components/SettingsRadioCardContainer.tsx +++ b/packages/twenty-front/src/modules/settings/components/SettingsRadioCardContainer.tsx @@ -25,16 +25,18 @@ export const SettingsRadioCardContainer = ({ onChange, }: SettingsRadioCardContainerProps) => { return ( - + {options.map((option) => ( ))} diff --git a/packages/twenty-front/src/modules/settings/security/components/SSO/SettingsSSOOIDCForm.tsx b/packages/twenty-front/src/modules/settings/security/components/SSO/SettingsSSOOIDCForm.tsx index 0a497f3e7..aa7837c22 100644 --- a/packages/twenty-front/src/modules/settings/security/components/SSO/SettingsSSOOIDCForm.tsx +++ b/packages/twenty-front/src/modules/settings/security/components/SSO/SettingsSSOOIDCForm.tsx @@ -30,6 +30,7 @@ const StyledLinkContainer = styled.div` const StyledButtonCopy = styled.div` align-items: end; display: flex; + margin-bottom: ${({ theme }) => theme.spacing(1)}; `; export const SettingsSSOOIDCForm = () => { @@ -70,6 +71,7 @@ export const SettingsSSOOIDCForm = () => { }); navigator.clipboard.writeText(authorizedUrl); }} + type="button" /> @@ -94,6 +96,7 @@ export const SettingsSSOOIDCForm = () => { }); navigator.clipboard.writeText(redirectionUrl); }} + type="button" /> diff --git a/packages/twenty-front/src/modules/settings/security/components/SSO/SettingsSSOSAMLForm.tsx b/packages/twenty-front/src/modules/settings/security/components/SSO/SettingsSSOSAMLForm.tsx index 9d0bbfcfb..3674a1d9f 100644 --- a/packages/twenty-front/src/modules/settings/security/components/SSO/SettingsSSOSAMLForm.tsx +++ b/packages/twenty-front/src/modules/settings/security/components/SSO/SettingsSSOSAMLForm.tsx @@ -52,6 +52,7 @@ const StyledLinkContainer = styled.div` const StyledButtonCopy = styled.div` align-items: end; display: flex; + margin-bottom: ${({ theme }) => theme.spacing(1)}; `; export const SettingsSSOSAMLForm = () => { @@ -136,6 +137,7 @@ export const SettingsSSOSAMLForm = () => { Icon={IconUpload} onClick={handleUploadFileClick} title={t`Upload file`} + type="button" > {isXMLMetadataValid() && ( { Icon={IconDownload} onClick={downloadMetadata} title={t`Download file`} - > + type="button" + /> @@ -181,6 +184,7 @@ export const SettingsSSOSAMLForm = () => { }); navigator.clipboard.writeText(acsUrl); }} + type="button" /> @@ -205,6 +209,7 @@ export const SettingsSSOSAMLForm = () => { }); navigator.clipboard.writeText(entityID); }} + type="button" /> diff --git a/packages/twenty-front/src/pages/settings/security/SettingsSecurityApprovedAccessDomain.tsx b/packages/twenty-front/src/pages/settings/security/SettingsSecurityApprovedAccessDomain.tsx index e1058b0d6..531268e05 100644 --- a/packages/twenty-front/src/pages/settings/security/SettingsSecurityApprovedAccessDomain.tsx +++ b/packages/twenty-front/src/pages/settings/security/SettingsSecurityApprovedAccessDomain.tsx @@ -23,7 +23,7 @@ export const SettingsSecurityApprovedAccessDomain = () => { const [createApprovedAccessDomain] = useCreateApprovedAccessDomainMutation(); - const formConfig = useForm<{ domain: string; email: string }>({ + const form = useForm<{ domain: string; email: string }>({ mode: 'onSubmit', resolver: zodResolver( z @@ -49,21 +49,18 @@ export const SettingsSecurityApprovedAccessDomain = () => { }, }); - const domain = formConfig.watch('domain'); + const domain = form.watch('domain'); const handleSave = async () => { try { - if (!formConfig.formState.isValid) { + if (!form.formState.isValid || !form.formState.isSubmitting) { return; } createApprovedAccessDomain({ variables: { input: { - domain: formConfig.getValues('domain'), - email: - formConfig.getValues('email') + - '@' + - formConfig.getValues('domain'), + domain: form.getValues('domain'), + email: form.getValues('email') + '@' + form.getValues('domain'), }, }, onCompleted: () => { @@ -86,67 +83,78 @@ export const SettingsSecurityApprovedAccessDomain = () => { }; return ( - navigate(SettingsPath.Security)} - onSave={formConfig.handleSubmit(handleSave)} - /> - } - links={[ - { - children: Workspace, - href: getSettingsPath(SettingsPath.Workspace), - }, - { - children: Security, - href: getSettingsPath(SettingsPath.Security), - }, - { children: New Approved Access Domain }, - ]} - > - -
- - ( - { - onChange(domain); - }} - fullWidth - placeholder="yourdomain.com" - error={error?.message} - /> - )} +
+ navigate(SettingsPath.Security)} + isSaveDisabled={form.formState.isSubmitting} /> -
-
- - ( - - )} - /> - {domain} -
- - + } + links={[ + { + children: Workspace, + href: getSettingsPath(SettingsPath.Workspace), + }, + { + children: Security, + href: getSettingsPath(SettingsPath.Security), + }, + { children: New Approved Access Domain }, + ]} + > + +
+ + ( + { + onChange(domain); + }} + fullWidth + placeholder="yourdomain.com" + error={error?.message} + /> + )} + /> +
+
+ + ( + + )} + /> + {domain} +
+ + + ); }; diff --git a/packages/twenty-front/src/pages/settings/security/SettingsSecuritySSOIdentifyProvider.tsx b/packages/twenty-front/src/pages/settings/security/SettingsSecuritySSOIdentifyProvider.tsx index d81da9b05..aaafd03c9 100644 --- a/packages/twenty-front/src/pages/settings/security/SettingsSecuritySSOIdentifyProvider.tsx +++ b/packages/twenty-front/src/pages/settings/security/SettingsSecuritySSOIdentifyProvider.tsx @@ -24,8 +24,8 @@ export const SettingsSecuritySSOIdentifyProvider = () => { const { enqueueSnackBar } = useSnackBar(); const { createSSOIdentityProvider } = useCreateSSOIdentityProvider(); - const formConfig = useForm({ - mode: 'onChange', + const form = useForm({ + mode: 'onSubmit', resolver: zodResolver(SSOIdentitiesProvidersParamsSchema), defaultValues: Object.values(sSOIdentityProviderDefaultValues).reduce( (acc, fn) => ({ ...acc, ...fn() }), @@ -35,12 +35,12 @@ export const SettingsSecuritySSOIdentifyProvider = () => { const handleSave = async () => { try { - const type = formConfig.getValues('type'); + const type = form.getValues('type'); await createSSOIdentityProvider( SSOIdentitiesProvidersParamsSchema.parse( pick( - formConfig.getValues(), + form.getValues(), Object.keys(sSOIdentityProviderDefaultValues[type]()), ), ), @@ -55,33 +55,34 @@ export const SettingsSecuritySSOIdentifyProvider = () => { }; return ( - navigate(SettingsPath.Security)} - onSave={handleSave} - /> - } - links={[ - { - children: Workspace, - href: getSettingsPath(SettingsPath.Workspace), - }, - { - children: Security, - href: getSettingsPath(SettingsPath.Security), - }, - { children: New SSO provider }, - ]} - > +
- + navigate(SettingsPath.Security)} + isSaveDisabled={form.formState.isSubmitting} + /> + } + links={[ + { + children: Workspace, + href: getSettingsPath(SettingsPath.Workspace), + }, + { + children: Security, + href: getSettingsPath(SettingsPath.Security), + }, + { children: New SSO provider }, + ]} + > + + - +
); }; diff --git a/packages/twenty-front/src/pages/settings/workspace/SettingsCustomDomain.tsx b/packages/twenty-front/src/pages/settings/workspace/SettingsCustomDomain.tsx index e76a2d3c7..a07420c3f 100644 --- a/packages/twenty-front/src/pages/settings/workspace/SettingsCustomDomain.tsx +++ b/packages/twenty-front/src/pages/settings/workspace/SettingsCustomDomain.tsx @@ -23,11 +23,7 @@ const StyledRecordsWrapper = styled.div` } `; -export const SettingsCustomDomain = ({ - handleSave, -}: { - handleSave: () => void; -}) => { +export const SettingsCustomDomain = () => { const { customDomainRecords, loading } = useRecoilValue( customDomainRecordsState, ); @@ -36,7 +32,7 @@ export const SettingsCustomDomain = ({ const { t } = useLingui(); - const { control, handleSubmit } = useFormContext<{ + const { control } = useFormContext<{ customDomain: string; }>(); @@ -57,11 +53,6 @@ export const SettingsCustomDomain = ({ onChange={onChange} placeholder="crm.yourdomain.com" error={error?.message} - onKeyDown={(e) => { - if (e.key === 'Enter') { - handleSubmit(handleSave); - } - }} loading={!!loading} fullWidth /> diff --git a/packages/twenty-front/src/pages/settings/workspace/SettingsCustomDomainRecords.tsx b/packages/twenty-front/src/pages/settings/workspace/SettingsCustomDomainRecords.tsx index 90048a4e1..bba788b03 100644 --- a/packages/twenty-front/src/pages/settings/workspace/SettingsCustomDomainRecords.tsx +++ b/packages/twenty-front/src/pages/settings/workspace/SettingsCustomDomainRecords.tsx @@ -7,9 +7,10 @@ import { TableHeader } from '@/ui/layout/table/components/TableHeader'; import { TableRow } from '@/ui/layout/table/components/TableRow'; import styled from '@emotion/styled'; import { useLingui } from '@lingui/react/macro'; -import { Button } from 'twenty-ui'; +import { Button, IconCopy } from 'twenty-ui'; import { useDebouncedCallback } from 'use-debounce'; import { CustomDomainValidRecords } from '~/generated/graphql'; +import { useTheme } from '@emotion/react'; const StyledTable = styled(Table)` border-bottom: 1px solid ${({ theme }) => theme.border.color.light}; @@ -42,12 +43,15 @@ export const SettingsCustomDomainRecords = ({ }) => { const { enqueueSnackBar } = useSnackBar(); + const theme = useTheme(); + const { t } = useLingui(); const copyToClipboard = (value: string) => { navigator.clipboard.writeText(value); enqueueSnackBar(t`Copied to clipboard!`, { variant: SnackBarVariant.Success, + icon: , }); }; @@ -67,6 +71,7 @@ export const SettingsCustomDomainRecords = ({ copyToClipboardDebounced(record.key)} + type="button" /> @@ -75,12 +80,14 @@ export const SettingsCustomDomainRecords = ({ onClick={() => copyToClipboardDebounced(record.type.toUpperCase()) } + type="button" /> copyToClipboardDebounced(record.value)} + type="button" /> diff --git a/packages/twenty-front/src/pages/settings/workspace/SettingsDomain.tsx b/packages/twenty-front/src/pages/settings/workspace/SettingsDomain.tsx index ef2e3e1c6..10bbf4a2b 100644 --- a/packages/twenty-front/src/pages/settings/workspace/SettingsDomain.tsx +++ b/packages/twenty-front/src/pages/settings/workspace/SettingsDomain.tsx @@ -107,6 +107,9 @@ export const SettingsDomain = () => { customDomain: customDomain && customDomain.length > 0 ? customDomain : null, }); + enqueueSnackBar(t`Custom domain updated`, { + variant: SnackBarVariant.Success, + }); }, onError: (error) => { if ( @@ -161,6 +164,10 @@ export const SettingsDomain = () => { subdomain, }); + enqueueSnackBar(t`Subdomain updated`, { + variant: SnackBarVariant.Success, + }); + redirectToWorkspaceDomain(currentUrl.toString()); }, }); @@ -169,12 +176,6 @@ export const SettingsDomain = () => { const handleSave = async () => { const values = form.getValues(); - if (!values || !form.formState.isValid || !currentWorkspace) { - return enqueueSnackBar(t`Invalid form values`, { - variant: SnackBarVariant.Error, - }); - } - if ( subdomainValue === currentWorkspace?.subdomain && customDomainValue === currentWorkspace?.customDomain @@ -184,6 +185,12 @@ export const SettingsDomain = () => { }); } + if (!values || !currentWorkspace) { + return enqueueSnackBar(t`Invalid form values`, { + variant: SnackBarVariant.Error, + }); + } + if ( isDefined(values.subdomain) && values.subdomain !== currentWorkspace.subdomain @@ -197,38 +204,40 @@ export const SettingsDomain = () => { }; return ( - Workspace, - href: getSettingsPath(SettingsPath.Workspace), - }, - { - children: General, - href: getSettingsPath(SettingsPath.Workspace), - }, - { children: Domain }, - ]} - actionButton={ - navigate(SettingsPath.Workspace)} - onSave={form.handleSubmit(handleSave)} - /> - } - > - - {/* eslint-disable-next-line react/jsx-props-no-spreading */} - - - {isCustomDomainEnabled && ( - <> - - - - )} - - - +
+ {/* eslint-disable-next-line react/jsx-props-no-spreading */} + + Workspace, + href: getSettingsPath(SettingsPath.Workspace), + }, + { + children: General, + href: getSettingsPath(SettingsPath.Workspace), + }, + { children: Domain }, + ]} + actionButton={ + navigate(SettingsPath.Workspace)} + isSaveDisabled={form.formState.isSubmitting} + /> + } + > + + + {isCustomDomainEnabled && ( + <> + + + + )} + + + +
); }; diff --git a/packages/twenty-front/src/pages/settings/workspace/SettingsSubdomain.tsx b/packages/twenty-front/src/pages/settings/workspace/SettingsSubdomain.tsx index 752e16324..dfc725c66 100644 --- a/packages/twenty-front/src/pages/settings/workspace/SettingsSubdomain.tsx +++ b/packages/twenty-front/src/pages/settings/workspace/SettingsSubdomain.tsx @@ -23,17 +23,13 @@ const StyledDomain = styled.h2` white-space: nowrap; `; -export const SettingsSubdomain = ({ - handleSave, -}: { - handleSave: () => void; -}) => { +export const SettingsSubdomain = () => { const domainConfiguration = useRecoilValue(domainConfigurationState); const { t } = useLingui(); const currentWorkspace = useRecoilValue(currentWorkspaceState); - const { control, handleSubmit } = useFormContext<{ + const { control } = useFormContext<{ subdomain: string; }>(); @@ -56,11 +52,6 @@ export const SettingsSubdomain = ({ error={error?.message} disabled={!!currentWorkspace?.customDomain} fullWidth - onKeyDown={(e) => { - if (e.key === 'Enter') { - handleSubmit(handleSave); - } - }} /> {isDefined(domainConfiguration.frontDomain) && ( diff --git a/packages/twenty-ui/src/input/button/components/Button.tsx b/packages/twenty-ui/src/input/button/components/Button.tsx index c80d4719a..536977168 100644 --- a/packages/twenty-ui/src/input/button/components/Button.tsx +++ b/packages/twenty-ui/src/input/button/components/Button.tsx @@ -408,18 +408,21 @@ export const Button = ({ soon = false, disabled = false, justify = 'flex-start', - focus = false, + focus: propFocus = false, onClick, to, target, dataTestId, hotkeys, ariaLabel, + type, }: ButtonProps) => { const theme = useTheme(); const isMobile = useIsMobile(); + const [isFocused, setIsFocused] = React.useState(propFocus); + return ( setIsFocused(true)} + onBlur={() => setIsFocused(false)} > {Icon && } {title} diff --git a/packages/twenty-ui/src/input/button/components/LightButton.tsx b/packages/twenty-ui/src/input/button/components/LightButton.tsx index 9fc383be8..25f2dadc2 100644 --- a/packages/twenty-ui/src/input/button/components/LightButton.tsx +++ b/packages/twenty-ui/src/input/button/components/LightButton.tsx @@ -14,6 +14,7 @@ export type LightButtonProps = { disabled?: boolean; focus?: boolean; onClick?: (event: MouseEvent) => void; + type?: React.ComponentProps<'button'>['type']; }; const StyledButton = styled.button< @@ -82,6 +83,7 @@ export const LightButton = ({ accent = 'secondary', disabled = false, focus = false, + type = 'button', onClick, }: LightButtonProps) => { const theme = useTheme(); @@ -91,6 +93,7 @@ export const LightButton = ({ onClick={onClick} disabled={disabled} focus={focus && !disabled} + type={type} accent={accent} className={className} active={active} diff --git a/packages/twenty-ui/src/input/components/Radio.tsx b/packages/twenty-ui/src/input/components/Radio.tsx index a9d760336..684116ce3 100644 --- a/packages/twenty-ui/src/input/components/Radio.tsx +++ b/packages/twenty-ui/src/input/components/Radio.tsx @@ -141,6 +141,7 @@ export const Radio = ({ id={optionId} name={name} data-testid="input-radio" + tabIndex={-1} checked={checked} value={value || label} radio-size={size}