- Sponsor
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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(medusa,types): create promotion flows #7029
Conversation
🦋 Changeset detectedLatest commit: d037de1 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
The latest updates on your projects. Learn more about Vercel for Git ↗︎
3 Ignored 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.
Looks pretty good overall 💪
I've added a few comments.
) | ||
|
||
return ( | ||
<Fragment key={index}> |
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.
suggestion: let fieldRile.id
be key here
zod.number().min(1, { message: "Required field" }), | ||
zod.string().min(1, { message: "Required field" }), | ||
zod.array(zod.string()).min(1, { message: "Required field" }), |
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.
todo: should translate the messages as well
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.
Can you do the translation outside the function? @fPolic
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.
yes:
import i18n from "i18next"
// ...
zod.string().min(1, { message: i18n.t("message.key")),
<Form.Item className="mb-2"> | ||
{fieldRule.required && ( | ||
<p className="text text-ui-fg-muted txt-small"> | ||
Required |
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.
todo: translate
ref={attributeRef} | ||
className="bg-ui-bg-base" | ||
> | ||
<Select.Value placeholder="Select Attribute" /> |
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.
todo: translate
<div className="absolute top-0 bottom-0 left-[40px] z-[-1] border-ui-border-strong w-px bg-[linear-gradient(var(--border-strong)_33%,rgba(255,255,255,0)_0%)] bg-[length:1px_3px] bg-repeat-y"></div> | ||
|
||
<Badge size="2xsmall" className=" text-xs"> | ||
AND |
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.
todo: translate
console.log("subKey - ", subKey) | ||
console.log("subValue - ", subValue) | ||
form.setValue(subKey, subValue) | ||
} | ||
} else { | ||
console.log("key - ", key) | ||
console.log("value - ", value) |
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.
todo: rm logs
<ProgressTabs | ||
value={tab} | ||
onValueChange={(tab) => handleTabChange(tab as Tab)} | ||
className="" |
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.
todo: rm
min={0} | ||
onValueChange={onChange} | ||
code={"USD"} | ||
symbol={getCurrencySymbol("USD")} |
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.
question: why is the amount set in USD...shouldn't this be disabled until currency_code
section is selected? If so we can disable the input and add a tooltip to explain
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.
Yes, you're right. This can't be done right now because we're accepting a multi select for currencies. There is a ticket in the followup that makes it just a singular currency, this will be changed when that is done.
targetRules, | ||
buyRules, | ||
campaigns, | ||
}) => { |
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.
nit: missing prop types
</div> | ||
)} | ||
|
||
<RulesFormField |
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.
suggestion: add currency codes to currency_code
dropdown
87a9a9e
to
6433781
Compare
221a3be
to
721f589
Compare
@fPolic good to merge? |
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.
Approved since the rest of the comments will be addressed in follow up tickets.
packages/admin-next/dashboard/src/v2-routes/promotions/common/edit-rules/edit-rules.tsx
Outdated
Show resolved
Hide resolved
const { campaigns } = useCampaigns() | ||
|
||
return ( | ||
<RouteFocusModal> |
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.
todo: there is x-axis scrollbar visible especially stands out when you turn on dark mode
…edit-rules/edit-rules.tsx Co-authored-by: Frane Polić <16856471+fPolic@users.noreply.github.com>
This PR currently has a merge conflict. Please resolve this and then re-add the |
what: