-
Notifications
You must be signed in to change notification settings - Fork 5.3k
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
[Components] snipcart #13200 #13877
base: master
Are you sure you want to change the base?
[Components] snipcart #13200 #13877
Conversation
WalkthroughThis pull request introduces several modules for managing discounts within the Snipcart application. It includes functionalities for creating, updating, and deleting discounts, along with a constants file for predefined options. Each module is structured to provide a clear interface for users, allowing for dynamic property generation based on user input. The enhancements facilitate comprehensive CRUD operations on discounts, improving the overall management capabilities within the application. Changes
Possibly related PRs
Poem
Recent review detailsConfiguration used: CodeRabbit UI Files ignored due to path filters (1)
Files selected for processing (2)
Additional context usedBiome
Additional comments not posted (3)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
3 Skipped Deployments
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
Outside diff range, codebase verification and nitpick comments (3)
components/snipcart/actions/delete-discount/delete-discount.mjs (1)
18-27
: Approve therun
method, suggest adding error handling.The method is implemented correctly and logs the operation's outcome effectively. However, consider adding error handling to manage potential failures in the
deleteDiscount
call.components/snipcart/actions/create-discount/create-discount.mjs (1)
70-121
: Approve the methods, suggest adding error handling.The methods are implemented correctly and provide dynamic functionality for creating discounts. Consider adding error handling to manage potential failures in the
createDiscount
call.components/snipcart/actions/update-discount/update-discount.mjs (1)
76-128
: Approve the methods, suggest adding error handling.The methods are implemented correctly and provide dynamic functionality for updating discounts. Consider adding error handling to manage potential failures in the
updateDiscount
call.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
pnpm-lock.yaml
is excluded by!**/pnpm-lock.yaml
Files selected for processing (5)
- components/snipcart/actions/create-discount/create-discount.mjs (1 hunks)
- components/snipcart/actions/delete-discount/delete-discount.mjs (1 hunks)
- components/snipcart/actions/update-discount/update-discount.mjs (1 hunks)
- components/snipcart/common/constants.mjs (1 hunks)
- components/snipcart/snipcart.app.mjs (1 hunks)
Files skipped from review due to trivial changes (1)
- components/snipcart/common/constants.mjs
Additional comments not posted (7)
components/snipcart/actions/delete-discount/delete-discount.mjs (2)
1-1
: Approved import statement.The import of
app
is correctly done from the relative path.
3-17
: Well-defined action properties.The action is properly defined with all necessary metadata and properties, including a helpful documentation link.
components/snipcart/actions/create-discount/create-discount.mjs (2)
1-1
: Approved import statement.The import of
app
is correctly done from the relative path.
3-69
: Well-defined action properties.The action is properly defined with all necessary metadata and properties, including a helpful documentation link. The conditional properties based on
trigger
andtype
are a sophisticated feature that enhances the flexibility of the action.components/snipcart/actions/update-discount/update-discount.mjs (2)
1-1
: Approved import statement.The import of
app
is correctly done from the relative path.
3-75
: Well-defined action properties.The action is properly defined with all necessary metadata and properties, including a helpful documentation link. The conditional properties based on
trigger
andtype
are a sophisticated feature that enhances the flexibility of the action.components/snipcart/snipcart.app.mjs (1)
7-64
: Well-structured property definitions.The new properties in
propDefinitions
are well-defined with clear types, labels, and descriptions. The use of constants for options in properties liketrigger
andtype
is a good practice as it enhances maintainability and readability.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Just one comment about the additionalProps
that applies to both create-discount
and update-discount
.
async additionalProps() { | ||
const props = {}; | ||
if (this.trigger === "Code") { | ||
props.code = { | ||
type: "string", | ||
label: "Code", | ||
description: "Code for the discount", | ||
}; | ||
} | ||
if (this.trigger === "Total") { | ||
props.totalToReach = { | ||
type: "string", | ||
label: "Total to Reach", | ||
description: "Minimum amount required to activate the discount", | ||
}; | ||
} | ||
if (this.type === "FixedAmount") { | ||
props.amount = { | ||
type: "string", | ||
label: "Amount", | ||
description: "Discount amount. Required when discount type is `FixedAmount`", | ||
}; | ||
} | ||
if (this.type === "Rate") { | ||
props.rate = { | ||
type: "string", | ||
label: "Rate", | ||
description: "Discount percentage, i.e.: `10`. Required when discount type is `Rate`", | ||
}; | ||
} | ||
return props; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since code
, totalToReach
, amount
, and rate
are already declared in the props section, I think you can just update their "hidden" and "disabled" values. (Applies to update-discount.mjs
as well).
async additionalProps() { | |
const props = {}; | |
if (this.trigger === "Code") { | |
props.code = { | |
type: "string", | |
label: "Code", | |
description: "Code for the discount", | |
}; | |
} | |
if (this.trigger === "Total") { | |
props.totalToReach = { | |
type: "string", | |
label: "Total to Reach", | |
description: "Minimum amount required to activate the discount", | |
}; | |
} | |
if (this.type === "FixedAmount") { | |
props.amount = { | |
type: "string", | |
label: "Amount", | |
description: "Discount amount. Required when discount type is `FixedAmount`", | |
}; | |
} | |
if (this.type === "Rate") { | |
props.rate = { | |
type: "string", | |
label: "Rate", | |
description: "Discount percentage, i.e.: `10`. Required when discount type is `Rate`", | |
}; | |
} | |
return props; | |
async additionalProps(existingProps) { | |
const props = {}; | |
if (this.trigger === "Code") { | |
existingProps.code.hidden = false; | |
existingProps.code.disabled: false, | |
} | |
if (this.trigger === "Total") { | |
existingProps.totalToReach.hidden = false; | |
existingProps.totalToReach.disabled = false; | |
} | |
if (this.type === "FixedAmount") { | |
existingProps.amount.hidden = false; | |
existingProps.amount.disabled = false; | |
} | |
if (this.type === "Rate") { | |
existingProps.rate.hidden = false; | |
existingProps.rate.disabled = false; | |
} | |
return props; |
Co-authored-by: michelle0927 <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like this is failing the lint check. Remember to run npx eslint --fix
on any changed files before pushing the changes.
The changes were implemented in create-discount
, but not update-discount
. Need to update update-discount
as well.
WHY
Summary by CodeRabbit
New Features
Improvements