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

refactor!: set up the login page as a starting page (DSP-1292) #370

Merged
merged 6 commits into from Jan 28, 2021

Conversation

@flavens
Copy link
Collaborator

@flavens flavens commented Jan 27, 2021

resolves DSP-1292

@flavens flavens self-assigned this Jan 27, 2021
@flavens flavens requested a review from kilchenmann Jan 27, 2021
@@ -1,3 +1,4 @@
<div class="login-page">
<dsp-login-form (loginSuccess)="refresh($event)"></dsp-login-form>
<dsp-login-form (loginSuccess)="refresh()"></dsp-login-form>
Copy link
Collaborator

@kilchenmann kilchenmann Jan 27, 2021

Choose a reason for hiding this comment

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

The LoginComponent emits a boolean value in loginSuccess. I do not understand why you removed it?

Loading

Copy link
Collaborator Author

@flavens flavens Jan 27, 2021

Choose a reason for hiding this comment

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

reversed in 27bc05c

Loading

if (this._session.getSession()) {
this._router.navigate(['dashboard']);
}
Copy link
Collaborator

@kilchenmann kilchenmann Jan 27, 2021

Choose a reason for hiding this comment

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

After successful login the user will always be redirected to the dashboard page, right?
Why not keeping this._router.navigate([this.returnUrl]);? In case a user bookmarked his project page e.g. admin.dasch.swiss/project/1111/info but (s)he's no longer logged-in, then it would be nice to be redirected to this page after logging-in.
But it's just a detail... maybe not important 😉

Loading

Copy link
Collaborator Author

@flavens flavens Jan 27, 2021

Choose a reason for hiding this comment

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

this is why I redirect explicitly to the dashboard, after logging in, I get this page (which I do not want):
Screenshot 2021-01-27 at 15 56 58

Loading

Copy link
Collaborator

@kilchenmann kilchenmann Jan 27, 2021

Choose a reason for hiding this comment

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

haha...yes true. The root path / was handled in the main component before. This component showed the landing page or the dashboard. Yes, as I said "it's just a little detail". I can live with it.
Another solution would have been to integrate the login page instead of the landing page. I.e. that still the main component handles the redirection.
But it's fine 😉

Loading

refresh(status: boolean) {
refresh() {
// check if a session is active
if (this._session.getSession()) {
Copy link
Collaborator

@kilchenmann kilchenmann Jan 27, 2021

Choose a reason for hiding this comment

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

could be if(status) {, because you get the status from login-form

Loading

Copy link
Collaborator Author

@flavens flavens Jan 27, 2021

Choose a reason for hiding this comment

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

refactored in 34e4acf

Loading

@flavens
Copy link
Collaborator Author

@flavens flavens commented Jan 27, 2021

@kilchenmann could you explain: this._componentCommsService.emit(new EmitEvent(Events.LoginSuccess, true)); from the refresh method?

Loading

@kilchenmann
Copy link
Collaborator

@kilchenmann kilchenmann commented Jan 27, 2021

@kilchenmann could you explain: this._componentCommsService.emit(new EmitEvent(Events.LoginSuccess, true)); from the refresh method?

I think this is what @mdelez implemented. It resolved DSP-331
This should be replaced by the new notification service from DSP-UI to keep the pop-ups consistent.

p.s. for dsp-admin you can comment it out.

Loading

@kilchenmann kilchenmann changed the title refactor(login): set up the login page as a starting page (DSP-1292) refactor!: set up the login page as a starting page (DSP-1292) Jan 28, 2021
@flavens flavens merged commit 46dfdbb into main Jan 28, 2021
4 checks passed
Loading
@flavens flavens deleted the wip/dsp-1292-login-landing-page branch Jan 28, 2021
@daschbot daschbot mentioned this pull request Jan 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants