saxifrage / cityasacampus

An open-source platform for connecting and showcasing resources within local learning communities.
http://cityasacampus.org/
5 stars 4 forks source link

logout functionality #371

Closed MatthewVita closed 8 years ago

MatthewVita commented 8 years ago

dev pittsburgh city as a campus explore

MatthewVita commented 8 years ago

@dmtroyer to do the code review

@cameronscott137 to review the above image to comment on where I placed the button and how it is styled (happy to make any changes)

dmtroyer commented 8 years ago

Code looks good to me!

!m @MatthewVita

MatthewVita commented 8 years ago

@dmtroyer yay. !m

Last check is @cameronscott137

dmtroyer commented 8 years ago

@timothyfcook may have an opinion regarding the styling

timothyfcook commented 8 years ago

A bit more padding inside the button. otherwise looks good!

chadwhitacre commented 8 years ago

I don't consistently get the logout button. For example, I don't get it when I hard-load the app on /#/dashboard.

screen shot 2015-12-07 at 5 37 14 pm

chadwhitacre commented 8 years ago

But if I load / I get it.

screen shot 2015-12-07 at 5 38 08 pm

But if I load /#/ I don't.

screen shot 2015-12-07 at 5 38 42 pm

But if I load /#/ and then navigate to /#/explore, I do.

screen shot 2015-12-07 at 5 39 14 pm

dmtroyer commented 8 years ago

I'm seeing the same behavior @whit537 mentions in https://github.com/saxifrage/cityasacampus/pull/371#issuecomment-162691668

MatthewVita commented 8 years ago

Okay all. I'll look into the bug and will change the logout design. Thanks

MatthewVita commented 8 years ago

found the issue: this line https://github.com/lynndylanhurley/ng-token-auth/blob/efe9660b31c348b05768fd9b35b23783a001c0cb/dist/ng-token-auth.js#L212 appears to be synchronous as it's just returning a boolean, but it's actually async because it calls the backend https://github.com/lynndylanhurley/ng-token-auth/blob/efe9660b31c348b05768fd9b35b23783a001c0cb/dist/ng-token-auth.js#L489

Not sure I like this; feels like a good candidate for returning a promise (will bring this up to the folks over at ng-token-auth repo to get their thoughts).

Regardless, I can sub on auth:validation-success and auth:validation-error to update the state for isAuthenticated method to return

bedtime ::zzz::

MatthewVita commented 8 years ago

UPDATE: I just added this commit https://github.com/saxifrage/cityasacampus/commit/8382565f96b5da9c1bd00ab40c0cdb03b624843e that fixes the isAuthenticated issue and applies the new styling.

@cameronscott137 2 things:

@dmtroyer Please review my latest commit. Basically the $auth.userIsAuthenticated function from ng-token is not the correct approach. Calling $auth.validateUser (which returns a promise) is more appropriate as it will make a backend call on each hard page load and just check for a token in memory on each angular route change.

chadwhitacre commented 8 years ago

Good debugging, @MatthewVita! :bug:

MatthewVita commented 8 years ago

@whit537 :)

img

MatthewVita commented 8 years ago

all squashed up and ready to go