Skip to content
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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

feat: focus ict errored field #478

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 9 additions & 3 deletions clients/banking/src/components/TransferInternationalDynamicFormBuilder.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ type DynamicForm = Form<Record<string, string>>;

export type DynamicFormApi = {
submitDynamicForm: DynamicForm["submitForm"];
setDynamicFieldError: DynamicForm["setFieldError"];
};

type TransferInternationalDynamicFormBuilderProps = {
Expand Down Expand Up @@ -92,9 +93,14 @@ type DynamicFormProps = {

const DynamicForm = forwardRef<DynamicFormApi, DynamicFormProps>(
({ fields, form, onChange }, forwardedRef) => {
const { Field, listenFields, submitForm, validateField, getFieldState } = useForm(form);

useImperativeHandle(forwardedRef, () => ({ submitDynamicForm: submitForm }), [submitForm]);
const { Field, listenFields, submitForm, validateField, getFieldState, setFieldError } =
useForm(form);

useImperativeHandle(
forwardedRef,
() => ({ submitDynamicForm: submitForm, setDynamicFieldError: setFieldError }),
[submitForm, setFieldError],
);

useEffect(() => {
const keys = Object.keys(form);
Expand Down
92 changes: 63 additions & 29 deletions clients/banking/src/components/TransferInternationalWizardBeneficiary.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,13 +9,19 @@ import { TransitionView } from "@swan-io/lake/src/components/TransitionView";
import { animations, colors } from "@swan-io/lake/src/constants/design";
import { useDebounce } from "@swan-io/lake/src/hooks/useDebounce";
import { useUrqlQuery } from "@swan-io/lake/src/hooks/useUrqlQuery";
import { isNotNullishOrEmpty, isNullishOrEmpty } from "@swan-io/lake/src/utils/nullish";
import {
isNotNullish,
isNotNullishOrEmpty,
isNullish,
isNullishOrEmpty,
} from "@swan-io/lake/src/utils/nullish";
import { useEffect, useMemo, useRef, useState } from "react";
import { ActivityIndicator, View } from "react-native";
import { hasDefinedKeys, useForm } from "react-ux-form";

import { AsyncData, Result } from "@swan-io/boxed";
import { LakeAlert } from "@swan-io/lake/src/components/LakeAlert";
import { LakeText } from "@swan-io/lake/src/components/LakeText";
import { noop } from "@swan-io/lake/src/utils/function";
import { StyleSheet } from "react-native";
import { P, match } from "ts-pattern";
Expand Down Expand Up @@ -50,7 +56,7 @@ export type Beneficiary = {
type Props = {
initialBeneficiary?: Beneficiary;
amount: Amount;
errors?: string[];
errors?: string[][];
Copy link
Contributor

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) => {

onPressPrevious: () => void;
onSave: (details: Beneficiary) => void;
};
Expand Down Expand Up @@ -81,25 +87,32 @@ export const TransferInternationalWizardBeneficiary = ({
[locale.language, dynamicFields],
);

const { Field, submitForm, FieldsListener, listenFields, setFieldValue, getFieldState } =
useForm<{
name: string;
route: string;
results: ResultItem[];
}>({
name: {
initialValue: initialBeneficiary?.name ?? "",
validate: validateRequired,
},
route: {
initialValue: initialBeneficiary?.route ?? "",
validate: () => undefined,
},
results: {
initialValue: initialBeneficiary?.results ?? [],
validate: () => undefined,
},
});
const {
Field,
submitForm,
FieldsListener,
listenFields,
setFieldValue,
getFieldState,
setFieldError,
} = useForm<{
name: string;
route: string;
results: ResultItem[];
}>({
name: {
initialValue: initialBeneficiary?.name ?? "",
validate: validateRequired,
},
route: {
initialValue: initialBeneficiary?.route ?? "",
validate: () => undefined,
},
results: {
initialValue: initialBeneficiary?.results ?? [],
validate: () => undefined,
},
});

useEffect(() => {
match(data)
Expand Down Expand Up @@ -147,16 +160,37 @@ export const TransferInternationalWizardBeneficiary = ({
listenFields(["results"], () => refresh());
}, [listenFields, refresh]);
Copy link
Contributor

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]);


useEffect(() => {
if (isNotNullish(errors) && errors.length) {
for (const error of errors) {
if (isNotNullishOrEmpty(error[0])) {
dynamicFormApiRef.current?.setDynamicFieldError(error[0], error[1]);
}
}
}
}, [setFieldError, errors, dynamicFormApiRef.current?.setDynamicFieldError]);
Copy link
Contributor

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,
);
Copy link
Contributor

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.


return (
<View>
<Tile>
{errors?.map((message, i) => (
<View key={`validation-alert-${i}`}>
<LakeAlert variant="error" title={message} />
<Space height={12} />
</View>
))}

<Tile
footer={
isNotNullish(orphanErrors) && orphanErrors.length ? (
Copy link
Contributor

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

<LakeAlert
anchored={true}
variant="error"
title={t("transfer.new.internationalTransfer.errors.title")}
>
{orphanErrors?.map((message, i) => (
<LakeText key={`validation-alert-${i}`}>{message}</LakeText>
))}
</LakeAlert>
) : null
}
>
Copy link
Contributor

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)

<LakeLabel
label={t("transfer.new.internationalTransfer.beneficiary.name")}
render={id => (
Expand Down
8 changes: 5 additions & 3 deletions clients/banking/src/components/TransferInternationalWizardDetails.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,12 +108,14 @@
graphQLErrors: P.array({
extensions: {
code: "BeneficiaryValidationError",
errors: P.array({ message: P.select(P.string) }),
errors: P.select(),
},
}),
},
([messages]) => {
onPressPrevious(messages);
([errors]) => {
onPressPrevious(
errors.map(({ message, path }) => [beneficiary.results[path[2]]?.key, message]),

Check failure on line 117 in clients/banking/src/components/TransferInternationalWizardDetails.tsx

View workflow job for this annotation

GitHub Actions / Test & build 

'errors' is of type 'unknown'.

Check failure on line 117 in clients/banking/src/components/TransferInternationalWizardDetails.tsx

View workflow job for this annotation

GitHub Actions / Test & build 

Binding element 'message' implicitly has an 'any' type.

Check failure on line 117 in clients/banking/src/components/TransferInternationalWizardDetails.tsx

View workflow job for this annotation

GitHub Actions / Test & build 

Binding element 'path' implicitly has an 'any' type.
);
},
)
.otherwise(() => showToast({ variant: "error", title: t("error.generic") }));
Expand Down
1 change: 1 addition & 0 deletions clients/banking/src/locales/en.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -727,6 +727,7 @@
"transfer.new.sendFullBalance.description": "Choose how much to leave in your account, and transfer the rest to your beneficiary",
"transfer.new.sendRegularTransfer": "Send a regular standing order instead",
"transfer.new.sendRegularTransfer.description": "You will send a regular standing order",
"transfer.new.internationalTransfer.errors.title": "Some error occured",
"transfer.new.internationalTransfer.title": "New international transfer",
"transfer.new.internationalTransfer.amount.title": "Enter details about your transfer",
"transfer.new.internationalTransfer.amount.summary.title": "You're sending",
Expand Down
Loading