-
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
[Payment history] add "Request early cancelation" menu item to the Subscription page (UI) #44763
[Payment history] add "Request early cancelation" menu item to the Subscription page (UI) #44763
Conversation
@rushatgabhane 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] |
src/pages/settings/Subscription/RequestEarlyCancellationPage/index.tsx
Outdated
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.
Some early review comments. Let me know once this is ready for review.
Screenshots seem to look pretty good. |
@mananjadhav PR updated, conflicts resolved! |
@dannymcclain right, the "Done" button's size was wrong, but after adding |
@JKobrynski Ok awesome, thanks! Let's just leave |
Reviewer Checklist
Screenshots/VideosAndroid: mWeb Chromemweb-chrome-subscription-cancellation.moviOS: mWeb Safarimweb-safari-subscription-cancelation.movMacOS: Chrome / Safariweb-subscription-cancelation-1.movweb-subscription-cancelation.movMacOS: Desktopdesktop-subscription-cancelation.mov |
I will review this pr and test it locally by the end of my work day today. |
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'm getting some weird behavior when I test this. The request returned "automatic" as the cancellation type but the UI shows the screen that should only show if the cancellation type is "manual". It looks like the onyx db is getting messed up somehow even though the request seems to be coming back with the correct data. @JKobrynski @mananjadhav are you not able to replicate this?
Screen.Recording.2024-07-22.at.2.48.48.PM.mov
Separately, can we add a loading indicator when the user hits "submit?" Having no immediate visual response when the user clicks "submit" feels weird. A loading indicator would be nice.
cancellationDate?: string; | ||
|
||
/** Cancellation reason */ | ||
cancellationReason?: FeedbackSurveyOptionID; |
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 don't think this should be optional. Lets see if we can refactor 👍.
cancellationReason?: FeedbackSurveyOptionID; | ||
|
||
/** Cancellation type (manual/automatic/none) */ | ||
cancellationType?: CancellationType; |
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 don't think this should be optional. Lets see if we can refactor 👍.
src/libs/actions/Subscription.ts
Outdated
{ | ||
errors: undefined, | ||
}, |
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 is causing issues with the onyx db. It looks like this might be adding a "errors: undefined" json object every time the api is called, regardless of whether the api is successful or not. Doesn't seem right...
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.
Right, I removed it
@blimpich regarding |
@blimpich about the video that you've posted, is it possible that you have an ongoing manual cancellation? I wasn't able to reproduce this on my end |
Lets discuss this in slack here: https://expensify.slack.com/archives/C036QM0SLJK/p1721774153958089
Same with this, separate slack thread here: https://expensify.slack.com/archives/C036QM0SLJK/p1721776484735149 |
And follow up to this comment, can we add a loading indicator when the user hits "submit?" Having no immediate visual response when the user clicks "submit" feels weird. A loading indicator would be nice. It will load for as long as it takes for the onyx data to come back and move us along to the next page. I think that should work? |
@blimpich would adding the loading spinner to the submit button work for what you're talking about? I feel like I've seen that pattern before and I think it would work well here for what we're trying to communicate ("Yes you've hit the button, we're working on it") |
@blimpich PR updated! I resolved both issues mentioned on slack, and I added a loading indicator to the form, let me know what you 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.
LGTM
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
🚀 Deployed to staging by https://github.com/blimpich in version: 9.0.12-0 🚀
|
🚀 Deployed to production by https://github.com/francoisl in version: 9.0.12-0 🚀
|
🚀 Deployed to staging by https://github.com/blimpich in version: 9.0.13-0 🚀
|
Details
Fixed Issues
$ #43282
PROPOSAL: N/A
Tests
Offline tests
N/A
QA Steps
Same as Tests section above
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
Automatic
chrome_automatic-compressed.webm
Manual
chrome_manual-compressed.webm
iOS: Native
iOS: mWeb Safari
Automatic
safari_automatic-compressed.mp4
Manual
safari_manual-compressed.mp4
MacOS: Chrome / Safari
Automatic
web_automatic-compressed.mov
Manual
web_manual-compressed.mov
MacOS: Desktop
Automatic
desktop_automatic-compressed.mov
Manual
desktop_manual-compressed.mov