Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Changes from 16 commits
5ae57d9
af7b6df
7881f60
2f96052
7da4c58
690d095
b2c9f6f
2ca5e3c
a5ad54a
06b74cc
f866282
8139378
e72b647
09b9538
9157519
4d7d132
11add28
f3cba59
6c5d56c
551789b
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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
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 tonvp_private_subscription.endDate
+ 1 monthThere 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).
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 and18
for a Control plan. I'm also asking in the doc if it should always be in USD or notThere 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
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.
Is there any reason to set this to
false
? It doesn't looks good on iOS without the safe area padding.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?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.
Maybe a better option than returning null would be to display
FullPageNotFoundView
?