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

New Donation Flow - Alerts and Summary Column #1314

Conversation

dimitur2204
Copy link
Contributor

@dimitur2204 dimitur2204 commented Feb 1, 2023

Closes #1286

Motivation and context

Form Improvements:

  • Added validation for the Login and Register forms
  • Moved validations and initial value objects to each respective step and part of the form
  • Moved the PaymentIntent update to run only once on submission
  • Changed form value names for the amount regarding card donation to make more sense
  • Added an error object on the DonationFlowContext
  • Fixed otherAmount field bug when there is an uneven amount of prices

Alerts column:

  • Added AlertsColumn which takes a sectionsRefArray which is an array of references. It then attaches alerts to each of the section references based on their id which is mapped to the [alerts](https://github.com/podkrepi-bg/frontend/pull/1314/files#diff-0b9f0d19b2035c89f509f8bf65f75cbc4ced3b1209c50b243ebd57ae86c2ebe9R39)
  • Added AnchoredAlert which is an alert that takes a sectionRef and is absolutely positioned with a top equal to section.offsetTop + spacing and titles on the section (theme.spacing(4) + 1.5rem)
  • Added PaymentSummaryAlert which is the summary of the payment, taxes and total of your payment.

Screenshots:

Desktop

Transaction Auth
image image

Mobile

Transaction Auth
image image

RFC

I think that this Anchoring approach might be problematic since React does not control refs and from my experience rerendering can be unpredictable.

If another viable approach comes to mind to anyone it would be good to hear @podkrepi-bg/frontend-maintainers

Affected urls

campaign/donation-v2/{slug}

@dimitur2204 dimitur2204 linked an issue Feb 2, 2023 that may be closed by this pull request
@dimitur2204 dimitur2204 self-assigned this Feb 2, 2023
@dimitur2204 dimitur2204 added type: RFC Request for comments area: donations Regarding something that includes the donation flow labels Feb 2, 2023
@dimitur2204 dimitur2204 marked this pull request as ready for review February 2, 2023 12:11
@dimitur2204 dimitur2204 added the run tests Allows running the tests workflows for forked repos label Feb 2, 2023
@github-actions github-actions bot removed the run tests Allows running the tests workflows for forked repos label Feb 2, 2023
Copy link
Member

@kachar kachar left a comment

Choose a reason for hiding this comment

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

Yet another swift update of the payment flow @dimitur2204

I see strong progress with this one.
Shared some ideas for code reusability and simplification.

Comment on lines 88 to 89
const { campaign, stripePaymentIntent, paymentError, setPaymentError } =
useContext(DonationFlowContext)
Copy link
Member

Choose a reason for hiding this comment

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

We can add a helper hook to get the context in a seamless way with:

export function useDonationFlow() {
  return useContext(DonationFlowContext)
}
Suggested change
const { campaign, stripePaymentIntent, paymentError, setPaymentError } =
useContext(DonationFlowContext)
const { campaign, stripePaymentIntent, paymentError, setPaymentError } = useDonationFlow()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in d45154f

src/components/donation-flow/steps/Amount.tsx Show resolved Hide resolved
src/components/donation-flow/DonationFlowForm.tsx Outdated Show resolved Hide resolved
children: (
<Box>
<Typography>Избирайки да се впишете. ще можете да:</Typography>
<List
Copy link
Member

Choose a reason for hiding this comment

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

We can extract this list as separate component to simplify the rendering of alerts

Copy link
Member

Choose a reason for hiding this comment

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

We might also expand the text and reduce the right padding/margin to contain more chars on single row

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed all on the list items, but kept the one on the X button row

@kachar
Copy link
Member

kachar commented Feb 2, 2023

We might improve the width of the different sections so they match, whichever width feels right for you, just to be consistent.

image

@kachar kachar closed this Feb 2, 2023
@kachar kachar reopened this Feb 2, 2023
@dimitur2204
Copy link
Contributor Author

We might improve the width of the different sections so they match, whichever width feels right for you, just to be consistent.

image

Matched the smaller width, by hardcoding it to the AnchoredAlert

@dimitur2204 dimitur2204 added the run tests Allows running the tests workflows for forked repos label Feb 2, 2023
@github-actions github-actions bot removed the run tests Allows running the tests workflows for forked repos label Feb 2, 2023
@dimitur2204 dimitur2204 merged commit 268a561 into podkrepi-bg:1272-epic-new-version-of-the-donation-flow Feb 2, 2023
@dimitur2204 dimitur2204 deleted the 1286-alerts-column-view-desktop branch February 2, 2023 21:04
@kachar
Copy link
Member

kachar commented Feb 3, 2023

Awesome update!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: donations Regarding something that includes the donation flow type: RFC Request for comments
Projects
None yet
Development

Successfully merging this pull request may close these issues.

New Donation Flow - Alerts Column on Desktop
4 participants