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

Jump to bottom

EP-2518 - Remove ConfirmationStep #1119

Merged
merged 8 commits into from
Dec 20, 2024
Merged

EP-2518 - Remove ConfirmationStep #1119

merged 8 commits into from
Dec 20, 2024

Conversation

caleballdrin
Copy link
Contributor

@caleballdrin caleballdrin commented Nov 20, 2024 •
edited
Loading

Description

There is a desire to remove step 2 of the branded checkout to reduce abandoned carts. This will mean the continue button on the first page should submit the gift and take the user to the thank you page.
https://jira.cru.org/browse/EP-2518

Changes I Made:

  • Move the submit logic to order.service and use it both in branded-checkout-step-1 and step-3
  • Conditionally show "Continue" or "Submit Your Gift" based on the useV3 attribute
  • Change next() page change based on the useV3 attribute
  • Add fullscreen fixed loading message
  • Move checkout-error-messages to a new component that is used both in branded-checkout-step-1 and step-3

@caleballdrin caleballdrin self-assigned this Nov 20, 2024
wrandall22
wrandall22 requested changes Nov 21, 2024
View reviewed changes
Copy link
Contributor

@wrandall22 wrandall22 left a comment

Choose a reason for hiding this comment

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

I still have a lot of digging to do, but figured I'd give an initial pass. Good job so far!

src/app/branded/step-1/branded-checkout-step-1.tpl.html Outdated Show resolved Hide resolved
src/app/branded/step-1/branded-checkout-step-1.tpl.html Outdated Show resolved Hide resolved
src/app/branded/step-1/branded-checkout-step-1.tpl.html Outdated Show resolved Hide resolved
src/common/components/Recaptcha/Recaptcha.tsx Outdated Show resolved Hide resolved
src/app/branded/step-1/branded-checkout-step-1.component.js Outdated Show resolved Hide resolved
src/common/components/Recaptcha/Recaptcha.tsx Outdated Show resolved Hide resolved
src/common/components/loading/loading.scss Show resolved Hide resolved
src/app/branded/step-1/branded-checkout-step-1.component.js Outdated Show resolved Hide resolved
src/app/branded/step-1/branded-checkout-step-1.component.js Show resolved Hide resolved
src/app/branded/step-1/branded-checkout-step-1.component.js Outdated Show resolved Hide resolved
wrandall22
wrandall22 requested changes Nov 26, 2024
View reviewed changes
Copy link
Contributor

@wrandall22 wrandall22 left a comment

Choose a reason for hiding this comment

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

I got a start on the review, a few minor things to change so far. Will try to get more done tomorrow.

src/common/services/api/order.service.spec.js Outdated Show resolved Hide resolved
src/common/services/api/order.service.spec.js Outdated Show resolved Hide resolved
src/common/services/api/order.service.spec.js Outdated Show resolved Hide resolved
src/app/checkout/step-3/step-3.component.spec.js Show resolved Hide resolved
src/app/checkout/step-3/step-3.component.spec.js Show resolved Hide resolved
src/common/services/api/order.service.spec.js Outdated Show resolved Hide resolved
src/common/services/api/order.service.spec.js Outdated Show resolved Hide resolved
src/common/services/api/order.service.spec.js Outdated Show resolved Hide resolved
src/common/services/api/order.service.spec.js Outdated Show resolved Hide resolved
@wrandall22 wrandall22 changed the title EP-2518 Remove ConfirmationStep EP-2518 - Remove ConfirmationStep Dec 5, 2024
wrandall22
wrandall22 requested changes Dec 5, 2024
View reviewed changes
src/app/branded/step-1/branded-checkout-step-1.component.js Outdated Show resolved Hide resolved
src/app/branded/step-1/branded-checkout-step-1.component.js Outdated Show resolved Hide resolved
src/common/services/api/order.service.js Outdated Show resolved Hide resolved
src/app/checkout/step-3/step-3.component.js Outdated Show resolved Hide resolved
src/common/services/api/order.service.js Outdated Show resolved Hide resolved
src/common/components/Recaptcha/RecaptchaWrapper.tsx Outdated Show resolved Hide resolved
src/common/components/nav/navCartIcon.component.spec.js Outdated Show resolved Hide resolved
src/app/branded/step-1/branded-checkout-step-1.component.js Outdated Show resolved Hide resolved
src/app/branded/step-1/branded-checkout-step-1.component.js Outdated Show resolved Hide resolved
src/app/branded/step-1/branded-checkout-step-1.component.js Show resolved Hide resolved
@caleballdrin caleballdrin force-pushed the EP-2518-remove-confirmation-step branch from e83ae91 to cef9728 Compare December 6, 2024 18:43
@wrandall22
Copy link
Contributor

I think this branch is good to resolve conflicts and do some squashing. Also, please put together a more robust PR description.

@caleballdrin caleballdrin force-pushed the EP-2518-remove-confirmation-step branch 2 times, most recently from 1a71b84 to e8792b0 Compare December 6, 2024 23:09
canac
canac reviewed Dec 9, 2024
View reviewed changes
Copy link
Contributor

@canac canac left a comment

Choose a reason for hiding this comment

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

This is looking really good! I mostly just have style feedback at this point.

src/app/checkout/checkout-error-messages/checkout-error-messages.tpl.html Outdated Show resolved Hide resolved
src/app/checkout/checkout-error-messages/checkout-error-messages.component.js Outdated Show resolved Hide resolved
src/app/branded/step-1/branded-checkout-step-1.component.js Outdated Show resolved Hide resolved
src/app/branded/step-1/branded-checkout-step-1.component.js Outdated Show resolved Hide resolved
src/app/branded/step-1/branded-checkout-step-1.component.js Outdated Show resolved Hide resolved
src/common/services/api/order.service.spec.js Outdated Show resolved Hide resolved
src/common/services/api/order.service.spec.js Outdated Show resolved Hide resolved
src/common/services/api/order.service.spec.js Outdated Show resolved Hide resolved
src/common/services/api/order.service.spec.js Outdated Show resolved Hide resolved
src/common/services/api/order.service.spec.js Outdated Show resolved Hide resolved
@caleballdrin caleballdrin force-pushed the EP-2518-remove-confirmation-step branch from 2e412a0 to 1b24d26 Compare December 19, 2024 02:06
@caleballdrin
Merge remote-tracking branch 'origin/EP-2517-branded-checkout-improve…
346ee7b 
…ments' into EP-2518-remove-confirmation-step
@caleballdrin
Copy link
Contributor Author

@wrandall22 can I merge this into EP-2517-branded-checkout-improvements ?

wrandall22
wrandall22 approved these changes Dec 19, 2024
View reviewed changes
Copy link
Contributor

@wrandall22 wrandall22 left a comment

Choose a reason for hiding this comment

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

I typically prefer not to have merge commits like 346ee7b, but the rest of the commits were well structured, good job!

@caleballdrin caleballdrin merged commit 405bd6e into EP-2517-branded-checkout-improvements Dec 20, 2024
1 check passed
@caleballdrin caleballdrin deleted the EP-2518-remove-confirmation-step branch December 20, 2024 02:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@canac canac canac left review comments

@wrandall22 wrandall22 wrandall22 approved these changes

Assignees

@caleballdrin caleballdrin

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@caleballdrin @wrandall22 @canac