Skip to content
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

9384 better error toast #9496

Open
wants to merge 4 commits into
base: develop
from

Conversation

Projects
None yet
4 participants
@jtj9817
Copy link

commented Mar 31, 2019

The old error handling method for the email address used only hard-coded strings which doesn't adequately handle other type of errors that are possible. The event.error.message method could possibly be a better solution but since I use a development build, there's no way to properly test it - addressed in the discussion of #9384 .

Fixes #9384 (hopefully)
The error message as it is right now:

jtj9817 added some commits Mar 31, 2019

Used FluxC's error message method
The old error handling method for the email address used only hardcoded strings which doesn't adequately handle other type of errors that are possible. event.error.message could possibly be a better solution
@peril-wordpress-mobile

This comment has been minimized.

Copy link

commented Mar 31, 2019

Warnings
⚠️ PR is not assigned to a milestone.

Generated by 🚫 dangerJS

jtj9817 added some commits Mar 31, 2019

@rachelmcr

This comment has been minimized.

Copy link
Member

commented Apr 16, 2019

Thanks for opening this PR, @jtj9817! I see there are some failing tests, though — have you had a chance to take a look at those failures?

@rachelmcr

This comment has been minimized.

Copy link
Member

commented Apr 16, 2019

@jtj9817 Actually, you can ignore my earlier message. I just had a check-in with @aforcier about this PR, and unfortunately I led you astray in my recommendations on #9384. I didn't consider that the error message coming from the server usually isn't localized, and we don't have a way to localize it from the app, so this solution won't work for all the languages we offer. I'm sorry about that!

@aforcier offered to look into this, since it may take a server-side fix. I'm going to go ahead and close this PR, but we really appreciate you digging in to this issue. Let us know if we can help you get started on another issue/fix!

@rachelmcr rachelmcr closed this Apr 16, 2019

@aforcier

This comment has been minimized.

Copy link
Contributor

commented Apr 25, 2019

@jtj9817 after some server-side digging, it seems like the best approach after all for the moment is to just use the error message string from the server as you had originally implemented. It won't cover every language used in the app, but at the moment it's a good compromise versus the server-side work that would need to happen.

Sorry for the trouble! I'm re-opening this PR. If you're up for it, please resolve the lint issues, and I believe a merge of develop into the branch is needed to resolve an errant submodule update.

@loremattei

This comment has been minimized.

Copy link
Contributor

commented May 6, 2019

I'm moving this to 12.5 since 12.4 has been cut. If this PR has to land 12.4, please feel free to target the release branch and to ping me to build a new beta.

@loremattei loremattei modified the milestones: 12.5 ❄️, 12.6 May 20, 2019

@loremattei

This comment has been minimized.

Copy link
Contributor

commented May 20, 2019

Hey! I'm moving this to 12.6 since 12.5 has been cut. If you want this to land 12.5, please feel free to target the release branch and ping me to build a new beta.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.