-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
[Refactor] Use CloseAccount
to replace User_Delete
and refactor related pages
#9635
Conversation
Looks like you modified Instead, all new API commands should use API.js, and follow our guidelines for writing new API commands. Unsure if your change is okay? Drop a note in #expensify-open-source! |
One problem to solve before marking ready for review: https://expensify.slack.com/archives/C02HWMSMZEC/p1656604993085699 |
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 LGTM
Thanks for the reviews so far gents! Comments addressed 👍 |
Needs an Onyx version bump before it will test well 👍 |
Please feel free to review with your morning coffee :) |
Looks great, thanks for the changes and discussion 🙇 |
package-lock.json
Outdated
"integrity": "sha1-BqZgTWpV1L9BqaR9mHLXp42jHnM=" | ||
"integrity": "sha512-xpkr6sCDIYTPqzvjG8M3ncw1YOTaloWZOyrUmicoEifBEKzQzt+ooUpRpQ/AbOoJfO/p2ZKiyp79qHThzJDulQ==" | ||
}, |
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.
@Beamanator are you on an older MBP? I think we agreed to stop commiting these changes unless you have an M1 MMP
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.
Oh interesting! I must have missed that - I'm on an Intel still, my M1 will be arriving during offshore 😅
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'll remove these unnecessary edits, thanks for calling this out
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 one question, but looking good
Updated @Julesssss 👍 |
wtf I didn't notice this didn't get merged 🙃 I'll fix conflicts today |
Conflicts fixed, got 2 previous approvals so if anyone wants to approve again we can merge |
✋ 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 @Julesssss in version: 1.1.87-0 🚀
|
🚀 Deployed to production by @yuwenmemon in version: 1.1.87-9 🚀
|
initWithStoredValues: false, | ||
closeAccount: { | ||
key: ONYXKEYS.CLOSE_ACCOUNT, | ||
initWithStoredValues: {error: '', isLoading: 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.
This is actually expecting a boolean and not the values
https://github.com/Expensify/react-native-onyx/blob/main/API.md#connectmapping--number
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.
ohhhhh interesting, good catch. It looks like in that repo we default initWithStoredValues
to true
so I think I can pretty much just delete this line - we do actually want to load the page with the error / loading state stored
cc @Julesssss since you worked on the Close Account project
Holding on related Web-E PR: https://github.com/Expensify/Web-Expensify/pull/34159Also holding on React-native-onyx version bump issue: https://github.com/Expensify/Expensify/issues/217767Details
See fixed issue for design doc ref
Fixed Issues
$ https://github.com/Expensify/Expensify/issues/213701
Tests (both Dev & Staging)
Same as mentioned in https://github.com/Expensify/Web-Expensify/pull/34159
PR Review Checklist
Contributor (PR Author) Checklist
### Fixed Issues
section aboveTests
sectionQA steps
sectiontoggleReport
and notonIconClick
)src/languages/*
filesSTYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)/** comment above it */
displayName
propertythis
properly so there are no scoping issues (i.e. foronClick={this.submit}
the methodthis.submit
should be bound tothis
in the constructor)this
are necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);
ifthis.submit
is never passed to a component event handler likeonClick
)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)Screenshots
Web & Desktop
VidCloseAccount.mov
Mobile Web
iOS
Android