-
-
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(dashboard): add campaign create to promotion UI #7306
Conversation
riqwan
commented
May 13, 2024
•
edited
Loading
edited
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.
How does it work if we have a promotion that is eligible for multiple currencies, but the campaign uses a single one, e.g. USD? I can't remember if we have talked about this previously.
If the promotion is configured for multiple currencies, perhaps a spend limit should be prohibited?
If I fill out the create campaign form, go back to promotion details, and then continue again to the campaign page, it seems all details are treated as unfilled in the form: CleanShot.2024-05-13.at.11.55.13.mp4 |
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.
LGTM! I added a few UI/UX suggestions.
|
||
<Form.Control> | ||
{isTypeSpend ? ( | ||
<CurrencyInput |
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: this should be disabled until currency is selected so we can display proper currency input (we usually add an info tooltip while on FormLabel in such cases)
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.
I tried this, but disabled doesn't seem to be working with currency input
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.
this disabled={!currencyValue}
should work if you pass after props spread {...props}
since it's probably overriding the prop
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.
in the new branch as well
<Form.Label>{t("campaigns.budget.fields.limit")}</Form.Label> | ||
|
||
<Form.Control> | ||
{isTypeSpend ? ( |
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: we should reset this value when we switch between usage limit and amount
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.
I tried this here:
useEffect(() => {
form.setValue(`budget.limit`, undefined)
}, [watchValueType])
But its only resetting every 2nd click, any reccos on how to go about this?
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.
hmm seems that input is having problem with undefined
, setting the value to ""
should do the trick:
form.setValue(`${fieldScope}budget.limit`, "")
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.
nice that worked, moved it to the new branch.