-
Notifications
You must be signed in to change notification settings - Fork 11
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
feat(Form fields): improve accesibility of errors #1246
Changes from 10 commits
c130ac1
13eb11f
6208fa2
11a6159
084e1ca
845e45d
91cfeda
4d9f72e
4a360d2
2afa5ba
6f480c3
634a69d
4e07e1a
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 |
---|---|---|
|
@@ -97,6 +97,7 @@ const CreditCardExpirationField = ({ | |
error, | ||
helperText, | ||
name, | ||
label, | ||
optional, | ||
validate: validateProp, | ||
onChange, | ||
|
@@ -112,9 +113,6 @@ const CreditCardExpirationField = ({ | |
const {setFormError, jumpToNext} = useForm(); | ||
|
||
const validate = (value: ExpirationDateValue, rawValue: string): string | undefined => { | ||
if (!rawValue) { | ||
return optional ? '' : texts.formFieldErrorIsMandatory || t(tokens.formFieldErrorIsMandatory); | ||
} | ||
Comment on lines
-115
to
-117
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've found this code repeated in most input fields, I've moved it inside |
||
const {month, year} = value; | ||
if (!month || !year) { | ||
return texts.formCreditCardExpirationError || t(tokens.formCreditCardExpirationError); | ||
|
@@ -142,6 +140,7 @@ const CreditCardExpirationField = ({ | |
|
||
const fieldProps = useFieldProps({ | ||
name, | ||
label, | ||
value, | ||
defaultValue, | ||
processValue, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -42,7 +42,6 @@ const TooltipContent = ({acceptedCards}: {acceptedCards: CardOptions}) => { | |
<Text2> | ||
{texts.formCreditCardCvvTooltipAmex || t(tokens.formCreditCardCvvTooltipAmex)} | ||
</Text2> | ||
) | ||
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. |
||
</Inline> | ||
)} | ||
</Stack> | ||
|
@@ -61,6 +60,7 @@ const CvvField = ({ | |
error, | ||
helperText, | ||
name, | ||
label, | ||
optional, | ||
validate: validateProp, | ||
onChange, | ||
|
@@ -79,9 +79,6 @@ const CvvField = ({ | |
const [isCvvHelpOpen, setIsCvvHelpOpen] = React.useState(false); | ||
|
||
const validate = (value: string, rawValue: string) => { | ||
if (!value) { | ||
return optional ? '' : texts.formFieldErrorIsMandatory || t(tokens.formFieldErrorIsMandatory); | ||
} | ||
if (value.length !== maxLength) { | ||
return texts.formCreditCardCvvError || t(tokens.formCreditCardCvvError); | ||
} | ||
|
@@ -92,6 +89,7 @@ const CvvField = ({ | |
|
||
const fieldProps = useFieldProps({ | ||
name, | ||
label, | ||
value, | ||
defaultValue, | ||
processValue, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,7 @@ | ||
'use client'; | ||
import * as React from 'react'; | ||
import {useTheme} from './hooks'; | ||
import * as tokens from './text-tokens'; | ||
|
||
export type FormStatus = 'filling' | 'sending'; | ||
export type FormErrors = {[name: string]: string | undefined}; | ||
|
@@ -9,6 +11,7 @@ export type FieldRegistration = { | |
input?: HTMLInputElement | HTMLSelectElement | null; | ||
validator?: FieldValidator; | ||
focusableElement?: HTMLDivElement | HTMLSelectElement | null; | ||
label?: string; | ||
}; | ||
|
||
type Context = { | ||
|
@@ -47,18 +50,21 @@ export const useForm = (): Context => React.useContext(FormContext); | |
|
||
export const useControlProps = <T,>({ | ||
name, | ||
label, | ||
value, | ||
defaultValue, | ||
onChange, | ||
disabled, | ||
}: { | ||
name: string; | ||
label?: string; | ||
value: undefined | T; | ||
defaultValue: undefined | T; | ||
onChange: undefined | ((value: T) => void); | ||
disabled?: boolean; | ||
}): { | ||
name: string; | ||
label?: string; | ||
value?: T; | ||
defaultValue?: T; | ||
onChange: (value: T) => void; | ||
|
@@ -77,11 +83,13 @@ export const useControlProps = <T,>({ | |
|
||
return { | ||
name, | ||
label, | ||
value, | ||
defaultValue: defaultValue ?? (value === undefined ? rawValues[name] ?? false : undefined), | ||
focusableRef: (focusableElement: HTMLDivElement | null) => | ||
register(name, { | ||
focusableElement, | ||
label, | ||
}), | ||
onChange: (value: T) => { | ||
setRawValue({name, value}); | ||
|
@@ -95,6 +103,7 @@ export const useControlProps = <T,>({ | |
|
||
export const useFieldProps = ({ | ||
name, | ||
label, | ||
value, | ||
defaultValue, | ||
processValue, | ||
|
@@ -108,6 +117,7 @@ export const useFieldProps = ({ | |
onChangeValue, | ||
}: { | ||
name: string; | ||
label: string; | ||
value?: string; | ||
defaultValue?: string; | ||
processValue: (value: string) => unknown; | ||
|
@@ -123,6 +133,7 @@ export const useFieldProps = ({ | |
value?: string; | ||
defaultValue?: string; | ||
name: string; | ||
label: string; | ||
helperText?: string; | ||
required: boolean; | ||
error: boolean; | ||
|
@@ -131,6 +142,7 @@ export const useFieldProps = ({ | |
inputRef: (field: HTMLInputElement | null) => void; | ||
onChange: (event: React.ChangeEvent<HTMLInputElement>) => void; | ||
} => { | ||
const {texts, t} = useTheme(); | ||
const {setRawValue, setValue, rawValues, values, formErrors, formStatus, setFormError, register} = | ||
useForm(); | ||
const rawValue = value ?? defaultValue ?? rawValues[name] ?? ''; | ||
|
@@ -151,15 +163,22 @@ export const useFieldProps = ({ | |
value, | ||
defaultValue: defaultValue ?? (value === undefined ? rawValues[name] ?? '' : undefined), | ||
name, | ||
label, | ||
helperText: formErrors[name] || helperText, | ||
required: !optional, | ||
error: error || !!formErrors[name], | ||
disabled: disabled || formStatus === 'sending', | ||
onBlur: (e: React.FocusEvent) => { | ||
setFormError({name, error: validate?.(values[name], rawValues[name])}); | ||
let error: string | undefined; | ||
if (!rawValues[name] && !optional) { | ||
error = texts.formFieldErrorIsMandatory || t(tokens.formFieldErrorIsMandatory); | ||
} else if (validate) { | ||
error = validate(values[name], rawValues[name]); | ||
} | ||
setFormError({name, error}); | ||
onBlur?.(e); | ||
}, | ||
inputRef: (input: HTMLInputElement | null) => register(name, {input, validator: validate}), | ||
inputRef: (input: HTMLInputElement | null) => register(name, {input, validator: validate, label}), | ||
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 need the field label to be in the registrations map, to be able to use it when rendering the global form error: "Revisa los errores en los siguientes campos: nombre, email". See changes in |
||
onChange: (event: React.ChangeEvent<HTMLInputElement>) => { | ||
const rawValue = event.currentTarget.value; | ||
const value = processValue(rawValue); | ||
|
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.
in desktop, the text is not aligned in the same way as mobile, @aweell is this expected?
is it possible to align the text line to the middle of the icon? (compare with mobile screenshot to see the difference)

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.
Not expected, but also not correctly documented in the specs
the size of the icon could be 1lh to match the line height in mobile (16) but also in desktop (20) so even if the font-size increases appears aligned to the first line of text? this approach seems correct?
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.
changed size to
1lh
(fallback to 1rem for old browsers not supportinglh
unit)