feat: add DiscountApplicationStrategyCard #154

mathiusj · 2023-09-18T23:00:51Z

What problem is this PR solving?

closes: https://github.com/Shopify/core-issues/issues/60529

Reviewers’ hat-rack 🎩

yarn storybook
review story for DiscountAppStrategyCard

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.

github-actions · 2023-09-18T23:01:06Z

Hello and thank you for your contribution! Before we can merge your pull request, we ask that you create a changeset to describe your changes. This will help us manage versions and release notes. You can create a changeset by running yarn changeset in your terminal. You can find more information about our contribution guidelines here. Thank you again!

locales/en.json

translation-platform · 2023-09-21T10:30:08Z

We have received a translation request but there is a question blocking translation work. Your help is needed.

Please answer the following questions.

Warning
Replies in GitHub are not visible to translators. Use the provided "Answer here" links. 🙅‍♀️

Could you please clarify? Should it mean "Only apply the first discount if the conditions are satisfied."? Answer here.

discount-app-components/locales/en.json

Line 565 in 01994a4

"helpText": "Only apply the first discount with conditions that are satisfied."

Note
Not your issue? Forward it to someone with more context; please don't leave it pending. 🙏
Questions? Hop in the #help-localization Slack channel.

devisscher

Looks good!

I'm reluctant to call it DiscountAppStrategyCard, it might be worth calling it DiscountApplicationStrategyCard even though it's longer :/ let me know how you feel about that. It would be more inline with the name of the api field and avoids confusion.

devisscher · 2023-09-21T13:06:12Z

src/components/DiscountAppStrategyCard/tests/DiscountAppStrategyCard.test.tsx

+
+    expect(methodCard).not.toContainReactComponent(TextField, {
+      label: 'Title',
+    });


Can we add a test that ensures the onChange function is called with the value of the choice selected?

devisscher · 2023-09-21T13:07:11Z

src/components/DiscountAppStrategyCard/README.md

+
+## Usage and best practices
+
+- The discount code length only adjusts the default length of generated codes, and does not set a max length for the text input


I don't think this part of the readme is related to this card?

devisscher · 2023-09-21T13:08:37Z

.changeset/gentle-windows-juggle.md

+'@shopify/discount-app-components': minor
+---
+
+Add DiscountAppStrategyCard


I'd add what it does here:

Add DiscountApplicationStrategyCard, which allows the user to select the discount application strategy, maximum or first.

thanks for the comments, have updated each of them in the latest commits 🙏

locales/en.json

devisscher

Looks good! Added a couple more comments. 😄

devisscher · 2023-09-21T19:15:51Z

src/components/DiscountApplicationStrategyCard/README.md

+### Basic usage
+
+```jsx
+<DiscountAppStategyCard strategy="All" />


Should be FIRST or MAXIMUM in the props. https://shopify.dev/docs/api/functions/reference/order-discounts/graphql/common-objects/discountapplicationstrategy :)

devisscher · 2023-09-21T19:16:29Z

src/types.ts

+
+export enum DiscountApplicationStrategy {
+  First = 'First',
+  Maximum = 'Maximum',


These values should be capitalized as per the API values: https://shopify.dev/docs/api/functions/reference/order-discounts/graphql/common-objects/discountapplicationstrategy

nice catch! updated

devisscher · 2023-09-21T19:26:32Z

src/components/DiscountApplicationStrategyCard/DiscountApplicationStrategyCard.tsx

+              },
+            ]}
+            selected={[strategy.value]}
+            onChange={handleChange}


One last thing sorry :D should the value be represented by the ENUM here? Instead of a translated string?

if you're referring specifically to the selected value, then I think it would be fine so long as the pattern in MethodCard is accurately representing the pattern we should value.

However, I do see that the choices' values can and should be updated to the enum!?

screenshot for reference:

Yes, I was referring to the value prop in the choices :) I guess my comment was at the wrong spot!

devisscher

🙌

mathiusj force-pushed the mathiusj/add-application-strategy-card branch 4 times, most recently from 327a8d2 to e76c803 Compare September 19, 2023 19:35

mathiusj changed the title ~~feat: add DiscountApplicationStrategyCard (wip)~~ feat: add DiscountApplicationStrategyCard Sep 19, 2023

mathiusj force-pushed the mathiusj/add-application-strategy-card branch from e76c803 to 01994a4 Compare September 19, 2023 19:36

mathiusj marked this pull request as ready for review September 19, 2023 19:36

translation-platform bot reviewed Sep 19, 2023

View reviewed changes

locales/en.json Show resolved Hide resolved

translation-platform bot reviewed Sep 19, 2023

View reviewed changes

locales/en.json Show resolved Hide resolved

mathiusj requested a review from devisscher September 19, 2023 19:37

devisscher reviewed Sep 21, 2023

View reviewed changes

mathiusj requested a review from devisscher September 21, 2023 17:42

mathiusj added 5 commits September 21, 2023 10:53

feat: add DiscountApplicationStrategyCard

56c676d

chore: update README

dd932d2

fix: use field instead of state, add test

0692df3

chore: update changeset

efb7fc1

chore: rename to DiscountApplicationStrategyCard

ee26d4a

translation-platform bot reviewed Sep 21, 2023

View reviewed changes

locales/en.json Show resolved Hide resolved

translation-platform bot reviewed Sep 21, 2023

View reviewed changes

locales/en.json Show resolved Hide resolved

mathiusj force-pushed the mathiusj/add-application-strategy-card branch from 12201a7 to ee26d4a Compare September 21, 2023 17:53

translation-platform bot reviewed Sep 21, 2023

View reviewed changes

locales/en.json Show resolved Hide resolved

translation-platform bot reviewed Sep 21, 2023

View reviewed changes

locales/en.json Show resolved Hide resolved

devisscher approved these changes Sep 21, 2023

View reviewed changes

fix: uppercase enum values

680499b

devisscher reviewed Sep 21, 2023

View reviewed changes

fix: update values to use enum instead of localized string

18b738c

mathiusj requested a review from devisscher September 21, 2023 19:32

chore: use or instead of and :D

7ac93b7

mathiusj added 2 commits September 21, 2023 12:37

chore: update changelog with all caps for enum vals

0612657

chore: use strategyField instead of string

fb2e43d

devisscher approved these changes Sep 21, 2023

View reviewed changes

mathiusj merged commit cb5c895 into main Sep 21, 2023
4 checks passed

mathiusj deleted the mathiusj/add-application-strategy-card branch September 21, 2023 19:59

shopify-github-actions-access bot mentioned this pull request Sep 21, 2023

Version Packages #153

Merged

translation-platform bot mentioned this pull request Sep 29, 2023

Update translations: #154 #163

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add DiscountApplicationStrategyCard #154

feat: add DiscountApplicationStrategyCard #154

mathiusj commented Sep 18, 2023 •

edited

Loading

github-actions bot commented Sep 18, 2023

translation-platform bot commented Sep 21, 2023 •

edited by mathiusj

Loading

devisscher left a comment

devisscher Sep 21, 2023

devisscher Sep 21, 2023

devisscher Sep 21, 2023

mathiusj Sep 21, 2023

devisscher left a comment

devisscher Sep 21, 2023

devisscher Sep 21, 2023

mathiusj Sep 21, 2023

devisscher Sep 21, 2023

mathiusj Sep 21, 2023 •

edited

Loading

devisscher Sep 21, 2023

mathiusj Sep 21, 2023

devisscher left a comment


		## Usage and best practices

		- The discount code length only adjusts the default length of generated codes, and does not set a max length for the text input

feat: add DiscountApplicationStrategyCard #154

feat: add DiscountApplicationStrategyCard #154

Conversation

mathiusj commented Sep 18, 2023 • edited Loading

What problem is this PR solving?

Reviewers’ hat-rack 🎩

Before requesting reviews

Before you deploy

github-actions bot commented Sep 18, 2023

translation-platform bot commented Sep 21, 2023 • edited by mathiusj Loading

devisscher left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devisscher left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mathiusj Sep 21, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devisscher left a comment

Choose a reason for hiding this comment

mathiusj commented Sep 18, 2023 •

edited

Loading

translation-platform bot commented Sep 21, 2023 •

edited by mathiusj

Loading

mathiusj Sep 21, 2023 •

edited

Loading