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
@W-17458039 - Handle error states for social/passwordless login and reset password #2185
base: feature-passwordless-social-login
Are you sure you want to change the base?
@W-17458039 - Handle error states for social/passwordless login and reset password #2185
Changes from 3 commits
b7cfad0
3857c96
60726cc
865765c
480a5f3
37f19c0
dd8f3e9
4917ef3
2531c5c
05eabf9
56b3de5
79116bb
812b92b
04e21fa
ee5e599
b13835d
ba3d8f4
8316458
d0344b6
95bea67
c555280
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.
Where do we based these messages on? I assume this is from API. What happens if the API adds/removes these messages?
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.
Yes these messages are from the API. If the error message does not match any of these, the error will show the generic
API_ERROR_MESSAGE
, which is "Something went wrong! Please try again."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.
my main concern is to have to maintain this list of constant. In a case where they change a bit of wording and an error will become obsolete.
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.
Looking at the spreadsheet of error states I wonder if instead of matching by error message regex we should base it on status code? It seems like it would be possible
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.
Would we consider moving the logic to throw the error message into commerce-sdk-react?
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.
We could... Is there a specific reason to move this to the sdk?
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 guess it would be to simplify the code within the retail react app, if we give the responsibility of throwing the error to the sdk the retail react app doesn't have to worry about checking the status code and can just catch the error when it occurs.
it'll also reduce code duplication, for example for passwordless checkout we wouldn't need to duplicate this logic
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.
+1 on moving it to the sdk