-
Notifications
You must be signed in to change notification settings - Fork 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
Early discount countdown banner #54901
base: main
Are you sure you want to change the base?
Early discount countdown banner #54901
Conversation
30b7483
to
e857669
Compare
e857669
to
498fc03
Compare
@eVoloshchak Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
Let us know when this is ready for design review! |
Can we get some screenshots for the design team please? |
🚧 @youssef-lr has triggered a test build. You can view the workflow run here. |
Added web. Adding more in sometime. |
🧪🧪 Use the links below to test this adhoc build on Android, iOS, Desktop, and Web. Happy testing! 🧪🧪 |
A couple things from the screenshots/videos so far:
Would love clarification about the offers and which should be dismissible, but the banner color in the chat needs to be updated regardless. |
That's correct, I believe I had it coded this way. @parasharrajat can you check? |
src/pages/settings/Subscription/CardSection/BillingBanner/EarlyDiscountBanner.tsx
Show resolved
Hide resolved
Added a suggestion in the code to fix that |
yes, I just enabled it from code for design team. |
ahh I see, okay cool then, let's keep it enabled until the design is good then we can revert. |
Should these banner be visible on mobile? I am not seeing any??? 🤔 |
Great catches Danny! Agree with all of that, and thanks for laying it all out so clearly. @parasharrajat I'm trying the test build with a new account and a new workspace, but I don't see the discount banner. Am I doing something wrong? |
You need to create a woekspace and then an expense to start free trial. I will add that to test steps. |
Yeah I agree—there's no reason to have that button there if you've already started a trial. What would it even do if you clicked on it?
Also not against this! But I don't feel strongly about it either. |
Maybe just me but I prefer it without, but don't feel strongly |
Bump @eVoloshchak |
All styles changes were made. |
…ediately after login
Updated videos are looking better, thanks! Seems like in some of the videos there's a |
For testing, I added the button forcefully so that you can see the whole UI. End result will have it based on the design. |
Awesome, thanks! |
Nice, yeah this is lookin' good! |
@parasharrajat eslint is failing |
@youssef-lr Those files are unrelated to this PR. |
Fixed the lint issue. |
Explanation of Change
Fixed Issues
$ #54817
PROPOSAL: #54817 (comment)
Tests
Claiming the first discount
Claiming the second discount
Period past 8 days
Offline tests
Same as QA steps.
QA Steps
Prerequisites:
Claiming the first discount
Go to your onboarding chat or concierge.
Verify you can see a countdown timer starting from 24h.
Verify the banner is not dismissable.
Verify that clicking on “Claim offer” takes you to the subscription page.
Verify the subscription page shows the same timer.
[Depends if backend has implemented the changes] Verify that after you add a card, a promoCode of 50% has been applied to your account.
Claiming the second discount
Period past 8 days
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)src/languages/*
files and using the translation methodSTYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)Design
label and/or tagged@Expensify/design
so the design team can review the changes.ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Android: Native
17.01.2025_15.26.54_REC.mp4
Android: mWeb Chrome
17.01.2025_15.29.45_REC.mp4
iOS: Native
17.01.2025_15.51.53_REC.mp4
iOS: mWeb Safari
17.01.2025_15.52.46_REC.mp4
MacOS: Chrome / Safari
17.01.2025_15.27.39_REC.mp4
MacOS: Desktop
17.01.2025_15.34.37_REC.mp4