-
Notifications
You must be signed in to change notification settings - Fork 25
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: focus ict errored field #478
Conversation
@@ -147,16 +160,37 @@ export const TransferInternationalWizardBeneficiary = ({ | |||
listenFields(["results"], () => refresh()); | |||
}, [listenFields, refresh]); |
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.
Don't forget to remove the listeners to avoid leaks / updating unmounted components states:
useEffect(() => {
const removeRouteListener = listenFields(["route"], ({ route: { value } }) => setRoute(value));
const removeResultsListener = listenFields(["results"], () => refresh());
return () => {
removeRouteListener();
removeResultsListener();
};
}, [listenFields, refresh]);
|
||
<Tile | ||
footer={ | ||
isNotNullish(orphanErrors) && orphanErrors.length ? ( |
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.
Avoid non-boolean values in conditions (orphanErrors.length
-> orphanErrors.length > 0
).
It often create issues, especially in react renders.
For example:
{orphanErrors.length && <p>Hello</p>} // will print the text "0" is array is empty
@@ -50,7 +56,7 @@ export type Beneficiary = { | |||
type Props = { | |||
initialBeneficiary?: Beneficiary; | |||
amount: Amount; | |||
errors?: string[]; | |||
errors?: string[][]; |
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.
I highly recommend using an array of tuples (with fixed size: 2). You can also default to a fixed instance of an empty array to avoid useless conditions in your components:
type ErrorsArray = Array<[name: string, error: string | undefined]>; // array of [string, string | undefined]
const NO_ERRORS: ErrorsArray = [];
type Props = {
initialBeneficiary?: Beneficiary;
amount: Amount;
errors?: ErrorsArray;
onPressPrevious: () => void;
onSave: (details: Beneficiary) => void;
};
export const TransferInternationalWizardBeneficiary = ({
initialBeneficiary,
amount,
errors = NO_ERRORS, // default to empty array
onPressPrevious,
onSave,
}: Props) => {
} | ||
} | ||
} | ||
}, [setFieldError, errors, dynamicFormApiRef.current?.setDynamicFieldError]); |
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.
Given the update above, you can safely do:
const setDynamicFieldError = dynamicFormApiRef.current?.setDynamicFieldError;
useEffect(() => {
for (const [name, error] of errors) {
setDynamicFieldError?.(name, error);
}
}, [errors, setDynamicFieldError]);
|
||
const orphanErrors = useMemo(() => errors?.filter(([key]) => isNullish(key)), [errors])?.map( | ||
([_, message]) => message, | ||
); |
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.
Could be removed entirely as there's no need to check if first tuple item is nullish.
</LakeAlert> | ||
) : null | ||
} | ||
> |
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.
Could be replaced with:
errors.length > 0 ? (
<LakeAlert
anchored={true}
variant="error"
title={t("transfer.new.internationalTransfer.errors.title")}
>
{errors.map(([_, message], i) => (
<LakeText key={`validation-alert-${i}`}>{message}</LakeText>
))}
</LakeAlert>
) : null
(no extra array)
@ncomont Is this still needed? |
don't merge this, please