-
Notifications
You must be signed in to change notification settings - Fork 23
Update $routeChangeError message #198
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
Update $routeChangeError message #198
Conversation
The current UX to alert the user with a popup when there is a routeChangeError is a bit aggressive and confusing to the user. I'm proposing to change this with a console log and redirecting to a 404 page.
Since is better tool than console.log in that it at least checks for browser support for console.log.
|
Can I get a CR for this, please? Thanking! |
|
@nancynaluz looks good to me! 👍 |
simonwalsh
left a comment
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 would prevent from using $log for this one since it won't output in production. This may mean that it will be harder for you or customer support to debug the issue.
My two cents! The rest looks good!
|
Is Travis expected to fail? |
|
@simonwalsh I didn't realize that @DanielWright I'm not sure.. but the other PRs seem to be failing, too. I wonder if master is failing with Travis, as well? |
|
@simonwalsh Only @nancynaluz Yeah, I was pretty sure the build wasn't green, just checking though. |
|
@DanielWright @simonwalsh Oh man, should I go back to |
|
As a matter of Angular 1.x convention, I'd say yes, go back to the injected service. |
To return back to Angular 1.0 conventions and since .info will continue to work in production.
|
@DanielWright LGTM? |
|
|
The current UX is to return the user with a popup when the data fails to load and there is a routeChangeError. This is a bit aggressive and confusing to the user. I'm proposing to change this to a log and redirect to 404 as seen in the PR that was merged to master here: sprangular#198
The current UX is to return the user with a popup when the data fails to load and there is a routeChangeError. This is a bit aggressive and confusing to the user. This commit changes this behaviour to logging a message in the console and redirecting to a 404 page, as seen in #198.
The current UX is to alert the user with a popup (an
alert) when a$routeChangeErroroccurs, which is a rather aggressive and confusing to the user.This PR proposes to change the
alertto aconsole.logand redirect to a 404 page.