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

EP-2517 Branded Checkout Improvements #1117

Open
wants to merge 14 commits into
base: master
Choose a base branch
from

Conversation

caleballdrin
Copy link
Contributor

@caleballdrin caleballdrin commented Nov 20, 2024

Base branch with use-v3 config option.

@caleballdrin caleballdrin self-assigned this Nov 20, 2024
<branded-checkout designation-number="2294554" show-cover-fees="true" ng-cloak></branded-checkout>
<script src="branded-checkout.v2.js"></script>
<branded-checkout designation-number="2294554" show-cover-fees="true" use-v3="true" ng-cloak></branded-checkout>
<script src="dev.v2.js"></script>
Copy link
Contributor

Choose a reason for hiding this comment

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

We'll want to change this back before we merge.

Choose a reason for hiding this comment

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

Was there mention of JSON config file for these options or did I make that up? Are we just using the use-v3 flag for everything?

Copy link
Contributor

Choose a reason for hiding this comment

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

There is no config file, we use this flag for everything.

@wrandall22
Copy link
Contributor

Thanks for getting this set up!

@caleballdrin caleballdrin force-pushed the EP-2517-branded-checkout-improvements branch from 46718e5 to 874f3fd Compare November 20, 2024 15:38
@caleballdrin
Copy link
Contributor Author

@wjames111 You can make your feature branches off of this one.

@wrandall22
Copy link
Contributor

Reminder to fix lint issues

Add styles for reordering gift selection with useV3 flag.
@wrandall22
Copy link
Contributor

Let's make sure to add notes in the Readme on this config and what it does.

@wrandall22
Copy link
Contributor

@wjames111 looks like the order switch of frequency/amount didn't take into account the new fields that show up:

Screenshot 2024-12-13 at 5 20 06 PM

@wjames111
Copy link

@wrandall22 hmm yeah that doesn't work. Could we just move frequency and amount before "Transaction Start Date for Reoccurring Gifts"?

@wrandall22
Copy link
Contributor

So, the current HTML order is Amount, Frequency, Dates; the desired order is Frequency, Dates, Amount (I think, though we should confirm this, because FL donate site has it as Frequency, Amount, Dates). So, if you achieve that with CSS or HTML restructure; either is okay, but CSS will be less duplicate code and less messy I think if you are able to figure out how to do it.

* Hides Middle Initial,  ‘Optional’ section, Additional your information header, Suffix,  Phone Number and  Spouse information based on useV3 flag.
@wjames111
Copy link

@caleballdrin how do you feel about putting this in stage?

ng-if="!$ctrl.loadingProductConfig && !$ctrl.errorLoadingProductConfig">
ng-if="!$ctrl.loadingProductConfig && !$ctrl.errorLoadingProductConfig"
use-v3="$ctrl.useV3"
>
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's put this > up on the line above.

Copy link
Contributor

Choose a reason for hiding this comment

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

We need to account for no phone number on the form when adding custom validators on v3.

@@ -65,7 +65,7 @@ <h4 class="panel-title border-bottom-small visible" translate>{{'YOUR_INFORMAT
</div>
</div>
</div>
<div class="col-sm-2">
<div ng-if="$ctrl.useV3 !== 'true'" class="col-sm-2">
Copy link
Contributor

Choose a reason for hiding this comment

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

The CSS should probably be updated to make first/last name look nicer in this case.

Screenshot 2024-12-18 at 2 04 21 PM

@@ -121,7 +121,7 @@ <h4 class="panel-title border-bottom-small visible" translate>{{'YOUR_INFORMAT

</div>

<div class="row" ng-if="$ctrl.donorDetails['donor-type'] === 'Household'">
<div class="row" ng-if="$ctrl.donorDetails['donor-type'] === 'Household' && $ctrl.useV3 !== 'true'">
Copy link
Contributor

Choose a reason for hiding this comment

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

The merge with staging and all the hideSpouseDetails config didn't quite go right.

@@ -239,7 +238,7 @@ <h4 class="panel-title border-bottom-small visible" translate>{{'CONTACT_INFO'
</div>
</div>
</div>
<div class="col-sm-6">
<div ng-if="$ctrl.useV3 !== 'true'" class="col-sm-6">
Copy link
Contributor

Choose a reason for hiding this comment

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

The styling should be updated for v3 to look nicer. I think we should do something about the h4 above in v3 as well. Email doesn't really belong with mailing address, but it feels weird all by itself as well under contact info.

Screenshot 2024-12-18 at 1 58 31 PM

@wjames111
Copy link

@caleballdrin @wrandall22 could we put all the BCO updates in here and get it on staging? That way we aren't solving merge conflicts twice with stage.

@wrandall22
Copy link
Contributor

wrandall22 commented Dec 18, 2024

Merges of BCO changes to staging should only come from this branch, not the various feature branches pointing to this one. However, not all of those branches are ready to migrate into this one yet. Well, changes related to v3 specifically.

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.

3 participants