-
-
Notifications
You must be signed in to change notification settings - Fork 182
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(organizations): code cleanup and patch data cleanup in account settings and tos view TASK-1292 #5290
base: main
Are you sure you want to change the base?
Conversation
// We're verifying that the user is logged in so we can consider the current account is from a valid logged user | ||
const {currentAccount} = useLocalObservable( | ||
() => { | ||
return {currentAccount: sessionStore.isLoggedIn && (sessionStore.currentAccount as AccountResponse)}; |
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 might just be a style preference, but it seems a little strange to me to have currentAccount here defined as false | AccountRespone
. Could we make it null instead of 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.
I left one comment, but otherwise lgtm
🗒️ Checklist
<type>(<scope>)<!>: <title> TASK-1234
frontend
orbackend
unless it's global👀 Preview steps
/me
endpoint. The payload should only contain the changed fields.preserve log
before saving data)💭 Notes
We're refactoring accountDetails and TOS view on the way of implementing some changes on displayed fields:
PATCH
request to update data