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

feat: add strategy all to StrategyCard #178

Merged
merged 4 commits into from
Nov 28, 2023
Merged

Conversation

mathiusj
Copy link
Contributor

@mathiusj mathiusj commented Nov 17, 2023

What problem is this PR solving?

closes #179

Reviewers’ hat-rack 🎩

  • yarn run storybook

Before
image

After for Product:
image

After for Shipping:

image

After for Order:

image

Before requesting reviews

Before you deploy

Warning
With every PR, you MUST think through each of the items in the following checklist and check the appropriate ones. This step cannot be overlooked by the PR author or its reviewers. Please remove this warning when you're done.

  • This PR is safe to rollback.
  • I tophatted this change on Storybook.

@@ -576,6 +576,10 @@
"maximum": {
"label": "Maximum",
"helpText": "Only apply the discount that offers the maximum reduction."
},
"all": {
"label": "All",
Copy link
Contributor

Choose a reason for hiding this comment

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

Localization quality issue found

The following issues may affect the quality of localized translations if they are not addressed:

  • The value All for key DiscountAppComponents.DiscountApplicationStrategyCard.all.label is very short. Short strings are more likely to be misunderstood by translators without context. Please provide additional context for the translators if possible.

Please look out for other instances of this issue in your PR and fix them as well if possible.

Questions about these messages? Hop in the #help-localization Slack channel.

@mathiusj mathiusj force-pushed the mathiusj/add-strategy-all branch from 830dcfb to d82c300 Compare November 17, 2023 20:15
Copy link

@samihibrahim samihibrahim left a comment

Choose a reason for hiding this comment

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

would this change the card for all discount types to include ALL? Because it is only supported for Product discounts - both order and shipping shouldn't have ALL as an option.

@mathiusj
Copy link
Contributor Author

would this change the card for all discount types to include ALL? Because it is only supported for Product discounts - both order and shipping shouldn't have ALL as an option.

hmm good point 🤔 I will look into how to best handle this!

Copy link
Contributor

@aeperea aeperea left a comment

Choose a reason for hiding this comment

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

lgtm!

@mathiusj
Copy link
Contributor Author

@aeperea @samihibrahim updated UI to conditionally render specific strategies based on discount class:

Order = First, Maximum
Product = First, Maximum, All
Shipping = All

@mathiusj mathiusj force-pushed the mathiusj/add-strategy-all branch from 3f6893f to 7a043f2 Compare November 21, 2023 18:43
const isShipping = discountClass === DiscountClass.Shipping;

const choices = [
...(isProduct || isOrder

Choose a reason for hiding this comment

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

❤️

Copy link

@samihibrahim samihibrahim left a comment

Choose a reason for hiding this comment

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

Looks Good Coder

Copy link
Contributor

@aeperea aeperea left a comment

Choose a reason for hiding this comment

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

LGTM! 🎉

@mathiusj mathiusj force-pushed the mathiusj/add-strategy-all branch from 71ed8bf to 243b4a2 Compare November 28, 2023 19:08
@mathiusj mathiusj force-pushed the mathiusj/add-strategy-all branch from 243b4a2 to 715459f Compare November 28, 2023 19:09
@mathiusj mathiusj merged commit 5f1021a into main Nov 28, 2023
4 checks passed
@mathiusj mathiusj deleted the mathiusj/add-strategy-all branch November 28, 2023 19:11
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.

[FEATURE] add strategy all to DiscountApplicationStrategyCard
3 participants