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.

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

Work in progress on AmountStep and InfoStep #934

Draft
wants to merge 26 commits into
base: feature/donate-wizard-react
Choose a base branch
from

Conversation

clarissalimab
Copy link
Contributor

This is a WIP for the migration of the Donate Wizard to React. It includes:

  • Storybook support with a few different scenarios;
  • Wizard styling;
  • AmountStep including custom amounts, or inserting a different amount, selecting recurring donation;
  • InfoStep including the supporter fields component, different payment methods and other details.

This is how it looks now:
donate-wizard

The style on InfoStep looks off because we're still not using the select input to choose the country and the state and the payment buttons need a review to check what's missing on the styles.

What's next:

  • Post the supporter when choosing a payment method;
  • Add designation on the AmountStep;
  • Missing components on the InfoStep;
    • CustomFields;
    • DedicationLink;
    • AnonymousCheckbox;
    • DedicationForm;
  • Fix the styles on the InfoStep;
  • Load countries and cities information on the InfoStep;
  • PaymentStep;
  • FollowupStep.

address: values.address,
next: stepManagerContext.next,
});
}} initialValues={{ supporter: props.supporter, address: props.address } as FormikFormValues}>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Important note: in React you should always set the form field default values; if the value is null or undefined, then it should be a blank string. React has problems tracking form field values when they're originally set to null or undefined (I don't totally understand it but every bit of advice has told me that).

If the service or whatever is getting the result of the submit expects that those values will be null or undefined, then you should convert empty form fields back to null or undefined on submit.

Comment on lines 126 to 137
<input
type="email"
name="email"
title={emailTitle}
required={props.required.email}
value={supporter?.email}
placeholder={emailTitle}
onChange={
(e) => {
setSupporterFields('email', e.target.value);
}
} />
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Handling input this way avoids using Formik entirely (or in the near future, react-hook-form) which isn't good because those tools give us things that help us. While it's likely not useful to investigate how Formik works at this point since I hope to have it switched over to react-hook-form today, the same principles apply there.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You mean it should be something like this?

Suggested change
<input
type="email"
name="email"
title={emailTitle}
required={props.required.email}
value={supporter?.email}
placeholder={emailTitle}
onChange={
(e) => {
setSupporterFields('email', e.target.value);
}
} />
<input
type="email"
name="email"
title={emailTitle}
required={props.required.email}
value={values.supporter.email}
placeholder={emailTitle}
onChange={
(e) => {
setFieldValue('supporter.email', e.target.value); // not sure if this is going to work
}
} />

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're talking about this right now :) Look at using the Field Component in Formik. 😄

amountOptions: Money[];
stateDispatch: (action: ActionType) => void;
currencySymbol: string;
singleAmount: string | null;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be Money | null. Everything else here uses Money so let's be consistent.

@wwahammy wwahammy marked this pull request as draft July 6, 2022 15:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants