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
Contributor

@flavens flavens commented Jan 27, 2021

resolves DSP-1292

@flavens flavens added the refactor Refactor or redesign label Jan 27, 2021
@flavens flavens self-assigned this 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
Contributor

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

reversed in 27bc05c

Comment on lines 33 to 35
if (this._session.getSession()) {
this._router.navigate(['dashboard']);
}
Copy link
Contributor

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 😉

Copy link
Contributor Author

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

Copy link
Contributor

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 😉

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

refactored in 34e4acf

@flavens
Copy link
Contributor Author

flavens commented Jan 27, 2021

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

@kilchenmann
Copy link
Contributor

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.

@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
@kilchenmann kilchenmann added the breaking Breaking Changes label Jan 28, 2021
@flavens flavens merged commit 46dfdbb into main Jan 28, 2021
@flavens flavens deleted the wip/dsp-1292-login-landing-page branch January 28, 2021 11:43
@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
Labels
breaking Breaking Changes refactor Refactor or redesign
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants