-
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
feat: Subscription settings UI #42990
feat: Subscription settings UI #42990
Conversation
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.
Left comments.
src/pages/settings/Subscription/SubscriptionSettingsSection.tsx
Outdated
Show resolved
Hide resolved
src/pages/settings/Subscription/SubscriptionSettingsSection.tsx
Outdated
Show resolved
Hide resolved
src/pages/settings/Subscription/SubscriptionSettingsSection.tsx
Outdated
Show resolved
Hide resolved
src/pages/settings/Subscription/SubscriptionSettingsSection.tsx
Outdated
Show resolved
Hide resolved
src/pages/settings/Subscription/SubscriptionSettingsSection.tsx
Outdated
Show resolved
Hide resolved
…sription-settings-ui
…sription-settings-ui
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 👍
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.
Just a few minor comments!
@Expensify/design would you like to review as well?
src/languages/en.ts
Outdated
subscriptionSettings: { | ||
title: 'Subscription settings', | ||
autoRenew: 'Auto-renew', | ||
yourAnnual: 'Your annual subscription will automatically renew on (date). You can switch to pay-per-use starting 30 days before renewal.', |
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.
(date)
should be a variable here, equal to nvp_private_subscription.endDate
+ 1 month
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.
Actually this translation key is not used anywhere 😅 It was present in the doc so I've added it but its not used anywhere so Im removing it
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 will however adjust the renewal date we display under auto-renew toggle
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.
Ah okay good catch, I think that initial text was from a previous iteration (I see it in the high level portion of the doc, but not Figma or the mocks in Detailed, so I think we're good to remove it).
src/languages/en.ts
Outdated
autoRenew: 'Auto-renew', | ||
yourAnnual: 'Your annual subscription will automatically renew on (date). You can switch to pay-per-use starting 30 days before renewal.', | ||
autoIncrease: 'Auto-increase annual seats', | ||
saveUpTo: 'Save up to $10/month per active member', |
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 needs to be slightly variable: it should be 10
for a Collect plan and 18
for a Control plan. I'm also asking in the doc if it should always be in USD or not
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.
Just gonna tag you here directly too @MitchExpensify 😄
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 think this should be localised to the billing currency, personally. It doesn't make sense if you're billed in GBP.
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.
At the moment all prices on main Subscription page have fixed $ currency. Do you want me to fix them all as part of this PR or do we leave the currency as it is for now and change it later in a separate issue?
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 think it's fine to be a follow-up, but let's make sure we do it.
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.
Ok, so should we create new issue for it? If so, who's job is it? :)
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.
Polish GH for later follow-up: #43277
return ( | ||
<ScreenWrapper | ||
testID={DisableAutoRenewSurveyPage.displayName} | ||
includeSafeAreaPaddingBottom={false} |
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.
Could this in theory still be accessed on native via deep link? Should we implement return null
for native?
It will be fixed with next rebase. Fix for it made it to main late afternoon :) |
@amyevans I've replied to/fixed whats been pointed out. Have a look when you can :) |
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.
1 small thing to go (my bad!), then we can merge!
// TODO these default state values will come from API in next phase | ||
const [autoRenew, setAutoRenew] = useState(true); | ||
const [autoIncrease, setAutoIncrease] = useState(false); | ||
|
||
// TODO this will come from API in next phase | ||
const expirationDate = format(new Date(), CONST.DATE.MONTH_DAY_YEAR_ABBR_FORMAT); | ||
const autoRenewalDate = privateSubscription?.endDate ? format(addMonths(new Date(privateSubscription?.endDate), 1), CONST.DATE.MONTH_DAY_YEAR_ABBR_FORMAT) : ''; |
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.
Sorry, I gave you bad info regarding adding a month to the end date. The autoRenewalDate should just be equal to the endDate (keep the formatting you've applied here though)
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.
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, checking
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.
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 think
const autoRenewalDate = privateSubscription?.endDate ? format(new Date(`${privateSubscription?.endDate}T00:00:00`), CONST.DATE.MONTH_DAY_YEAR_ABBR_FORMAT) : '';
would work, currently it's picking up my local timezone I think
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.
Ok, I've changed it. I will be finishing for today but on monday I might get back to this (in this or one of next PRs) and refactor it to something more elegant. I believe getNewSubscriptionRenewalDate
might have the same TZ issue so I will look into it
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.
Okay, I'm going to merge this PR to keep things moving but definitely feel free to refactor in a follow up PR. Thank you!
src/pages/settings/Subscription/DisableAutoRenewSurveyPage/index.native.tsx
Show resolved
Hide resolved
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
Only UI was implemented in this PR. Submit action will come in the next phase - there is a comment that serves as a reminder as well.
It might be the case that it's being already implemented in one of the PRs. |
🚀 Deployed to production by https://github.com/luacmartins in version: 1.4.81-11 🚀
|
<FormAlertWithSubmitButton | ||
isAlertVisible={shouldShowReasonError} | ||
onSubmit={handleSubmit} | ||
message="common.error.pleaseCompleteForm" |
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.
Error message here was left hardcoded, it needed translate function. #44075
Details
Fixed Issues
$ #38626
PROPOSAL:
Tests
/settings/subscription
Offline tests
QA Steps
/settings/subscription
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.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
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
MacOS: Desktop