-
-
Notifications
You must be signed in to change notification settings - Fork 165
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
WIP: feat: add option to hide connected user panel #483
base: master
Are you sure you want to change the base?
WIP: feat: add option to hide connected user panel #483
Conversation
ping @AnthonyNahas |
thanks for contributing I will check that asap |
It seems angular doesn't support |
b4b1a78
to
9d4e1c1
Compare
Refactored the fix :) It's now ready to review |
projects/ngx-auth-firebaseui/src/lib/components/ngx-auth-firebaseui/auth.component.ts
Outdated
Show resolved
Hide resolved
<button (click)="signOut()" class="sign-out-button action-button" color="warn" | ||
mat-stroked-button>{{ signOutText }}</button> | ||
</div> | ||
<div *ngIf="isUserProfileScreenVisible(user)" class="signed-in-container" fxLayout="column" fxLayoutAlign="center center" *ngIf=""> |
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.
can u please provide more info about the implementation and why the logic should change ?
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.
In current implementation, only two "alternatives" are covered:
- If the user is signed In: we display the profile or if the email is not verified (and guard exists) then we display the panel telling the user must validate its email.
- If the user is not signed in: we display the form
My proposal is to offer a third option (implementation detail: I moved the condition in a method so the HTML is more readable).
To allow this third option i removed the if ; else
syntax, thus removing the ng-template. After my change we basically have 3 options:
- If the user is not connected: display the form
- If the user is connected and doesn't have email validated (and a guard exists) we display the email validation screen
- If the user is connected and have email validated but for a reason I don't want, where I placed the form, to display the "profile" data (picture, logout button etc), then we add a flag to allow not to display this profile. This "profile" view replacing the form after login may disturb the design.
Since everything has parameters in ngx-auth-firebaseui, it's nice to allow developper not to display something and allowing him to use the new ngx-auth-firebaseui with signin/register/email validation guard without forcing display of profile after login.
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 will perform more tests to find if there is another way to achieve this :)
<a [routerLink]="goBackURL" class="go-back-button action-button" color="primary" | ||
mat-stroked-button>{{ verifyEmailGoBackText }}</a> | ||
<button (click)="signOut()" class="sign-out-button action-button" color="warn" | ||
mat-stroked-button>{{ signOutText }}</button> |
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.
same here
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.
see above for detail :)
@@ -85,6 +86,7 @@ export class AuthComponent implements OnInit, AfterViewInit, OnChanges, OnDestro | |||
// tslint:disable-next-line:no-output-on-prefix | |||
@Output() onError: any; | |||
@Output() selectedTabChange: EventEmitter<MatTabChangeEvent> = new EventEmitter(); | |||
@Output() loading: EventEmitter<boolean> = new EventEmitter(); |
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.
why do we need a new event emitter ?
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.
the new event emitter was added in case we want to alert the container component something inside is loading. Can be useful to display a loader over the whole container. It doesn't disturb the flow. I can remove it though
Any progress on this issue? |
can u please adapt the PR to the latest version? |
Sometimes especially when using onSuccess callbacks, we don't want to show the connected user profile panel or the email confirmation panel.
Since success can be async and execute some tasks before being handled, it can for example create visual glitches. E.g. redirecting after a third party REST call -> we see the profile briefly before being redirected.