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

WIP: feat: add option to hide connected user panel #483

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

blackholegalaxy
Copy link
Contributor

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.

@blackholegalaxy
Copy link
Contributor Author

ping @AnthonyNahas

@blackholegalaxy blackholegalaxy changed the title feat: add option to hide connected user panel WIP: feat: add option to hide connected user panel Mar 24, 2020
@AnthonyNahas
Copy link
Owner

thanks for contributing

I will check that asap

@blackholegalaxy
Copy link
Contributor Author

It seems angular doesn't support | async as xxx along with another condition so i must find a way to add the flag elsewhere

@blackholegalaxy blackholegalaxy force-pushed the feat/add-option-to-hide-connected-user-panel branch from b4b1a78 to 9d4e1c1 Compare March 25, 2020 00:02
@blackholegalaxy blackholegalaxy changed the title WIP: feat: add option to hide connected user panel feat: add option to hide connected user panel Mar 25, 2020
@blackholegalaxy
Copy link
Contributor Author

Refactored the fix :) It's now ready to review

.circleci/config.yml 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="">
Copy link
Owner

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 ?

Copy link
Contributor Author

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.

Copy link
Contributor Author

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>
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here

Copy link
Contributor Author

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();
Copy link
Owner

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 ?

Copy link
Contributor Author

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

@blackholegalaxy blackholegalaxy changed the title feat: add option to hide connected user panel WIP: feat: add option to hide connected user panel Mar 28, 2020
@Blunderchips
Copy link
Contributor

Any progress on this issue?

@AnthonyNahas
Copy link
Owner

can u please adapt the PR to the latest version?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants