-
Notifications
You must be signed in to change notification settings - Fork 21
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
2499: Add coupon banner #2558
2499: Add coupon banner #2558
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## feature/2459-campaign-creation-flow #2558 +/- ##
===================================================================
Coverage 63.8% 63.8%
===================================================================
Files 326 326
Lines 5089 5089
Branches 1231 1231
===================================================================
Hits 3245 3245
Misses 1676 1676
Partials 168 168
Flags with carried forward coverage won't be shown. Click here to find out more. |
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.
Thanks all. I've left some feedback inline, but the main issue is that this isn't displaying on the fourth step when testing locally.
I'll get some clarity about whether the useFreeAdCredit()
hook needs to be adjusted, but if so, we'll handle that as a separate task. For now, the new banner should display any time the current "Claim $500 in ads credit..." text was previously shown and that text should be removed.
js/src/setup-mc/setup-stepper/setup-paid-ads/paid-ads-features-section.js
Show resolved
Hide resolved
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 is working as expected. I left a comment inline about needing to address the requirements for when this should be displayed in a follow-up task.
js/src/setup-mc/setup-stepper/setup-paid-ads/paid-ads-features-section.js
Outdated
Show resolved
Hide resolved
QA/Test Report- ✅Testing Environment -
Test Results - Ad coupon banner appears as described in the description. ✅ Functional Demo / Screencast - |
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.
Thanks for the work. There are two issues need to be resolved.
js/src/setup-mc/setup-stepper/setup-paid-ads/paid-ads-features-section.js
Outdated
Show resolved
Hide resolved
@joemcgill I made a few updates following @eason9487 's comments. Can you kindly check please? |
@eason9487 I've opened #2599 to clarify and address the requirements for the |
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.
Thanks for the further update! LGTM.
188983d
into
feature/2459-campaign-creation-flow
Changes proposed in this Pull Request:
Closes #2499
Replace this with a good description of your changes & reasoning.
Screenshots:
Detailed test instructions:
Additional details:
Changelog entry