Skip to content

Conversation

@nancynaluz
Copy link
Contributor

The current UX is to alert the user with a popup (an alert) when a $routeChangeError occurs, which is a rather aggressive and confusing to the user.

This PR proposes to change the alert to a console.log and redirect to a 404 page.

nancynaluz added 2 commits December 12, 2016 15:07
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.
@nancynaluz
Copy link
Contributor Author

Can I get a CR for this, please? Thanking!

@gen035
Copy link

gen035 commented Dec 14, 2016

@nancynaluz looks good to me! 👍

Copy link
Contributor

@simonwalsh simonwalsh left a 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!

@DanielWright
Copy link

Is Travis expected to fail?

@nancynaluz
Copy link
Contributor Author

@simonwalsh I didn't realize that $log wouldn't output to production :/ I thought that it was a wrapper of sorts to use console.log() if the browser supports it! I can change it.

@DanielWright I'm not sure.. but the other PRs seem to be failing, too. I wonder if master is failing with Travis, as well?

@DanielWright
Copy link

@simonwalsh Only $log.debug() calls are disabled in production. $log.log will still get written to the console.

@nancynaluz Yeah, I was pretty sure the build wasn't green, just checking though.

@nancynaluz
Copy link
Contributor Author

nancynaluz commented Dec 14, 2016

@DanielWright @simonwalsh Oh man, should I go back to $log.log()? 😅

@DanielWright
Copy link

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.
@nancynaluz
Copy link
Contributor Author

@DanielWright LGTM?

@DanielWright
Copy link

:shipit:

@DanielWright DanielWright merged commit b34250d into sprangular:master Dec 14, 2016
nancynaluz pushed a commit to nancynaluz/sprangular that referenced this pull request Dec 19, 2016
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
DanielWright pushed a commit that referenced this pull request Dec 21, 2016
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants