-
Notifications
You must be signed in to change notification settings - Fork 56
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
fix: Identity error on billing change modal #2659
fix: Identity error on billing change modal #2659
Conversation
This is suppose to use the global identity error that was added in #2623. |
Hmm okay, I thought the issue was with the message, not the error handling. Got it! Fixing 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 is working correctly if the user does not have an identity loaded in the browser at all, but is not working for when the user has the wrong identity loaded in the browser.
You can reproduce this by logging in using pictl and updating the user's identity with the pictl userupdatekey
command. The identity saved to the browser will then be incorrect.
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 code works. But the error is outside the modal and the only way I can leave the Error page is returning to the Home page.
I think this is not the greatest UX. Can we keep the error inside the Modal? I'm not sure if ErrorBoundaries work with React Portal so the ideal solution might not be using them.
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.
Approved before by mistake.
@tiagoalvesdulce this was defined on #2623. Maybe it's outside the pr scope... Let's open an issue and discuss better solutions for it, what do you think? |
Let's use the global error for now so that we can get the release pushed then open an issue for further improving this UX. |
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.
tACK
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. @victorgcramos please create the issue so we don't forget to improve it.
Closes #2656.
Fixes Identity error message on Set Billing Status modal.
UI Changes
Before:
After: