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-2521 Standard Buttons instead of Radio Buttons for suggested amounts #1116

Merged

Conversation

rguinee
Copy link
Contributor

@rguinee rguinee commented Nov 19, 2024

EP-2521

Use Buttons instead of Radio Buttons for suggested amounts.

Adjusted the ProductConfigForm file to be buttons instead of inputs. Rearranged the amount and label controllers to suit the Figma designs. Added CSS to style the buttons and add interactivity when tapping/clicking an option.

@rguinee rguinee requested a review from j2trumpet November 19, 2024 21:54
@@ -68,48 +68,54 @@ <h4 class="panel-title border-bottom-small" translate>
<h4 class="panel-title border-bottom-small" translate>
{{'GIFT_AMOUNT'}}
</h4>
<div class="panel-body pt0">
<div class="panel-body p0">
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 need to make sure of 2 things from stakeholders:

  1. Should this change affect mobile only?
  2. Should this change be tied to the new v3 config option?

<div class="form-group">
<div class="button-group">
<div
role="button"
Copy link
Contributor

Choose a reason for hiding this comment

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

Why did you move away from an actual button element?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great question. I had started a response while at the dentist last week and that is now gone. Initially, I opted to use a div for more control of the element but I'm realizing that we'll need to declare some accessibility functionality of each element that is inherit in a button element. But, I may end up going all the way back to using input element with radio type and just styling it to match our current design. Thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think a styled radio input would work fine, but I'm not fully read-in on this task, so I've assumed there was some reason to switch from radio inputs to buttons.

@j2trumpet
Copy link
Contributor

For the suggested amount inputs, the design changes (amount background gets darker and height increases) when the user clicks on it, but these changes don't happen for the custom amount until the user enters a value. Could these changes happen when the user clicks/focuses on the custom input field for the sake of consistent UX?

@rguinee
Copy link
Contributor Author

rguinee commented Dec 6, 2024 via email

@rguinee
Copy link
Contributor Author

rguinee commented Dec 9, 2024

For the suggested amount inputs, the design changes (amount background gets darker and height increases) when the user clicks on it, but these changes don't happen for the custom amount until the user enters a value. Could these changes happen when the user clicks/focuses on the custom input field for the sake of consistent UX?

@j2trumpet I've made this change. See here.

@j2trumpet
Copy link
Contributor

I don't know what the ultimate goal for this PR/ticket is, but hopefully sticking with the radio inputs and just styling them differently works. @wrandall22 ?

From a UX/UI standpoint, I think we're good.

@wrandall22
Copy link
Contributor

Here is what I see on my phone.

Screenshot_20241210_084626_Chrome.jpg

Screenshot_20241210_084839_Chrome.jpg

@wrandall22
Copy link
Contributor

The main purpose is to make it easier to select these amounts on mobile devices, which is accomplished with this for sure. However, we still need to determine from stakeholders if this is going to be tied to v3 and if it will be tied specifically to mobile.

@wrandall22
Copy link
Contributor

Please change the target branch to EP-2517-branded-checkout-improvements since this will be tied to v3. Also, we need to add some conditionals to make sure this only happens on v3.

@rguinee rguinee changed the base branch from master to EP-2517-branded-checkout-improvements December 11, 2024 16:13
@rguinee
Copy link
Contributor Author

rguinee commented Dec 11, 2024

Please change the target branch to EP-2517-branded-checkout-improvements since this will be tied to v3. Also, we need to add some conditionals to make sure this only happens on v3.

Updated target branch. I'll need assistance on those conditionals.

@wrandall22
Copy link
Contributor

You can look at https://github.com/CruGlobal/give-web/pull/1120/files for the ng-if statements as a starting point.

@rguinee
Copy link
Contributor Author

rguinee commented Dec 11, 2024 via email

@wrandall22
Copy link
Contributor

Let's make sure to clean up this Git history so the commits from the base branch don't show up here. Also, the productConfigForm conflicts need to be resolved.

@rguinee rguinee changed the title EP-2521 Standard Buttons instead of Radio Buttons for suggested amounts Resolving conflicts in productConfigForm Dec 16, 2024
@rguinee rguinee changed the title Resolving conflicts in productConfigForm Merge branch 'EP-2517-branded-checkout-improvements' into EP-2521-Radio-Button-redesign-1 Dec 16, 2024
@rguinee rguinee changed the title Merge branch 'EP-2517-branded-checkout-improvements' into EP-2521-Radio-Button-redesign-1 EP-2521 Standard Buttons instead of Radio Buttons for suggested amounts Dec 16, 2024
@rguinee rguinee force-pushed the EP-2521-Radio-Button-redesign-1 branch from 60989d7 to 79fcbcb Compare January 8, 2025 17:01
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.

It looks like there is still quite a bit of cleanup to do in the git history. What I would recommend at this point, since basically all the work is done in the first commit, is to squash everything into one commit, then do a git reset head^ to uncommit everything, then make individual commits bringing in specific changes that are logical. For this particular PR, probably 2 or 3 commits would be sufficient, something like:

  1. Added margin to Gift Amount header (and why)
  2. Build out custom amount buttons for branded checkout version 3
  3. Style the new layout

name="amount"
class="form-control form-control-subtle"
type="text"
id="ca"
Copy link
Contributor

Choose a reason for hiding this comment

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

This line and line 178 are removed in the next commit. This is where a squash would come into play.

</li>
</ul>
</div><!-- // panel-body -->
</div><!-- // panel -->
Copy link
Contributor

Choose a reason for hiding this comment

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

This change is undone in the next commit, should be squashed.

ng-change="$ctrl.changeStartDay($ctrl.itemConfig['RECURRING_DAY_OF_MONTH'], $ctrl.itemConfig['RECURRING_START_MONTH'])">
</select>
</label>
</div>
Copy link
Contributor

Choose a reason for hiding this comment

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

This change (removal of comment on div) is undone in the next commit, should be squashed.

@@ -146,7 +233,9 @@ <h4 class="panel-title border-bottom-small" translate>
</div>
</div>
</div>
</div><!-- // panel -->
Copy link
Contributor

Choose a reason for hiding this comment

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

This change (removal of comment on div) should be squashed.

Comment on lines 171 to 131
<input
name="suggestedAmount"
type="radio"
ng-checked="$ctrl.customInputActive"
/>
<input
name="amount"
class="form-control form-control-subtle"
type="text"
id="ca"
step="1"
tabindex="-1"
ng-model="$ctrl.customAmount"
ng-change="$ctrl.changeCustomAmount($ctrl.customAmount)"
ng-focus="$ctrl.customInputActive = true;"
ng-required="$ctrl.customInputActive"
placeholder="{{'OTHER_PLACEHOLDER' | translate}}" />
<input name="suggestedAmount"
Copy link
Contributor

Choose a reason for hiding this comment

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

These changes aren't related to ng-if and should be squashed into the previous commit.

@@ -629,6 +629,32 @@ input[type="radio"] {
}
/* Product Config Form Amounts End */

.radio-method,
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems to be the main change in this commit, so the commit message should reflect what is happening here.

@@ -187,6 +187,6 @@ export default angular
onOrderFailed: '&',
language: '@',
showCoverFees: '@',
useV3: '@'
useV3: '@',
Copy link
Contributor

Choose a reason for hiding this comment

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

This commit should just go away since the only change is removed in the next commit.

</div>
<loading inline="true" ng-if="$ctrl.changingFrequency" class="mt-- text-center">
<translate>{{'CHANGING_FREQUENCY'}}</translate>
</loading>
</div><!-- // panel-body -->
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks like it should be squashed into the first or second commit.

</div><!-- // panel -->
</div><!-- // panel-body -->
</div><!-- // panel -->
</div>
</div>
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks like it should be squashed into the first or second commit.

</div>
</div>
<div class="panel panel-default give-modal-panel">
<div ng-if="$ctrl.useV3 !== 'true'" class="panel panel-default give-modal-panel">
Copy link
Contributor

Choose a reason for hiding this comment

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

This is unrelated to your PR, so this change should be squashed with the first commit so it will just disappear.

@rguinee rguinee closed this Jan 9, 2025
@rguinee rguinee force-pushed the EP-2521-Radio-Button-redesign-1 branch from 79fcbcb to e5bb559 Compare January 9, 2025 18:09
@rguinee rguinee deleted the EP-2521-Radio-Button-redesign-1 branch January 9, 2025 18:14
@rguinee rguinee restored the EP-2521-Radio-Button-redesign-1 branch January 10, 2025 20:32
@rguinee rguinee reopened this Jan 10, 2025
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.

Great job!

@rguinee rguinee merged commit 72d00e3 into EP-2517-branded-checkout-improvements Jan 15, 2025
1 check passed
@rguinee rguinee deleted the EP-2521-Radio-Button-redesign-1 branch January 15, 2025 15:43
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