From 0526b9a624812d480b214a8bdbe8268b9709713c Mon Sep 17 00:00:00 2001 From: ccremer Date: Fri, 10 Mar 2023 08:56:13 +0100 Subject: [PATCH 1/4] Navigate back via history, not via relative parent link This should give a greater UX since whenever the user clicks the close button, they expect to be taken where they come from, and not to the relative parent page. Especially if linked from another component entirely. --- src/app/app.module.ts | 4 ++- .../billingentity-view.component.html | 2 +- .../organization-edit.component.html | 2 +- .../organization-members-edit.component.html | 2 +- src/app/shared/back-link.directive.ts | 14 +++++++++ src/app/shared/navigation.service.ts | 30 +++++++++++++++++++ src/app/shared/shared.module.ts | 4 ++- .../teams/team-edit/team-edit.component.html | 2 +- src/app/zones/zone/zone-detail.component.html | 2 +- 9 files changed, 55 insertions(+), 7 deletions(-) create mode 100644 src/app/shared/back-link.directive.ts create mode 100644 src/app/shared/navigation.service.ts diff --git a/src/app/app.module.ts b/src/app/app.module.ts index 64efaae9..36526e76 100644 --- a/src/app/app.module.ts +++ b/src/app/app.module.ts @@ -37,6 +37,7 @@ import { OrganizationCollectionService } from './store/organization-collection.s import { KubernetesDataServiceFactory } from './store/kubernetes-data.service'; import { KubernetesCollectionServiceFactory } from './store/kubernetes-collection.service'; import { SelfSubjectAccessReviewCollectionService } from './store/ssar-collection.service'; +import { NavigationService } from './shared/navigation.service'; @NgModule({ declarations: [ @@ -72,7 +73,8 @@ import { SelfSubjectAccessReviewCollectionService } from './store/ssar-collectio SelfSubjectAccessReviewCollectionService, { provide: APP_INITIALIZER, - deps: [AppConfigService, OAuthService], + // start the NavigationService early to catch route events. + deps: [AppConfigService, OAuthService, NavigationService], useFactory: initializeAppFactory, multi: true, }, diff --git a/src/app/billingentity/billingentity-view/billingentity-view.component.html b/src/app/billingentity/billingentity-view/billingentity-view.component.html index 87cf6a4b..fb276147 100644 --- a/src/app/billingentity/billingentity-view/billingentity-view.component.html +++ b/src/app/billingentity/billingentity-view/billingentity-view.component.html @@ -4,7 +4,7 @@
{{ billingEntity.metadata.name }}
- +
diff --git a/src/app/organizations/organization-edit/organization-edit.component.html b/src/app/organizations/organization-edit/organization-edit.component.html index b352cb8b..b3518777 100644 --- a/src/app/organizations/organization-edit/organization-edit.component.html +++ b/src/app/organizations/organization-edit/organization-edit.component.html @@ -8,7 +8,7 @@ {{ payload.organization.metadata.name }}
- + diff --git a/src/app/organizations/organization-members-edit/organization-members-edit.component.html b/src/app/organizations/organization-members-edit/organization-members-edit.component.html index 568d040b..29b24160 100644 --- a/src/app/organizations/organization-members-edit/organization-members-edit.component.html +++ b/src/app/organizations/organization-members-edit/organization-members-edit.component.html @@ -6,7 +6,7 @@ {{ payload.members.metadata.namespace }} Members - + diff --git a/src/app/shared/back-link.directive.ts b/src/app/shared/back-link.directive.ts new file mode 100644 index 00000000..a1f1b89a --- /dev/null +++ b/src/app/shared/back-link.directive.ts @@ -0,0 +1,14 @@ +import { Directive, HostListener } from '@angular/core'; +import { NavigationService } from './navigation.service'; + +@Directive({ + selector: '[appBackLink]', +}) +export class BackLinkDirective { + constructor(private navigation: NavigationService) {} + + @HostListener('click') + onClick(): void { + this.navigation.back(); + } +} diff --git a/src/app/shared/navigation.service.ts b/src/app/shared/navigation.service.ts new file mode 100644 index 00000000..f471a841 --- /dev/null +++ b/src/app/shared/navigation.service.ts @@ -0,0 +1,30 @@ +import { Injectable } from '@angular/core'; +import { Location } from '@angular/common'; +import { NavigationEnd, Router } from '@angular/router'; + +/** + * See https://nils-mehlhorn.de/posts/angular-navigate-back-previous-page/ + */ +@Injectable({ providedIn: 'root' }) +export class NavigationService { + private history: string[] = []; + + constructor(private router: Router, private location: Location) { + this.router.events.subscribe((event) => { + if (event instanceof NavigationEnd) { + this.history.push(event.urlAfterRedirects); + } + }); + } + + back(): void { + const record = this.history.pop(); + if (this.history.length > 0) { + this.location.back(); + } else { + window.history.replaceState(null, '', '/'); + window.history.pushState(null, '', record ?? this.router.url); + this.location.back(); + } + } +} diff --git a/src/app/shared/shared.module.ts b/src/app/shared/shared.module.ts index 9f9fe14b..d56cb6f9 100644 --- a/src/app/shared/shared.module.ts +++ b/src/app/shared/shared.module.ts @@ -20,9 +20,10 @@ import { MessageModule } from 'primeng/message'; import { ConfirmDialogModule } from 'primeng/confirmdialog'; import { MultiSelectModule } from 'primeng/multiselect'; import { ProgressSpinnerModule } from 'primeng/progressspinner'; +import { BackLinkDirective } from './back-link.directive'; @NgModule({ - declarations: [], + declarations: [BackLinkDirective], imports: [], exports: [ CommonModule, @@ -47,6 +48,7 @@ import { ProgressSpinnerModule } from 'primeng/progressspinner'; ConfirmDialogModule, MultiSelectModule, ProgressSpinnerModule, + BackLinkDirective, ], }) export class SharedModule {} diff --git a/src/app/teams/team-edit/team-edit.component.html b/src/app/teams/team-edit/team-edit.component.html index 6473c453..8f594aed 100644 --- a/src/app/teams/team-edit/team-edit.component.html +++ b/src/app/teams/team-edit/team-edit.component.html @@ -9,7 +9,7 @@ {{ team.metadata.name }} - + diff --git a/src/app/zones/zone/zone-detail.component.html b/src/app/zones/zone/zone-detail.component.html index cddafe00..41b100ef 100644 --- a/src/app/zones/zone/zone-detail.component.html +++ b/src/app/zones/zone/zone-detail.component.html @@ -41,7 +41,7 @@ From 5c954168a8269a860f1c9c43f203b5539031c611 Mon Sep 17 00:00:00 2001 From: ccremer Date: Mon, 13 Mar 2023 14:47:38 +0100 Subject: [PATCH 2/4] Limit history size To prevent memory increasing forever in long sessions --- src/app/shared/navigation.service.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/src/app/shared/navigation.service.ts b/src/app/shared/navigation.service.ts index f471a841..41be674c 100644 --- a/src/app/shared/navigation.service.ts +++ b/src/app/shared/navigation.service.ts @@ -13,6 +13,7 @@ export class NavigationService { this.router.events.subscribe((event) => { if (event instanceof NavigationEnd) { this.history.push(event.urlAfterRedirects); + this.history.splice(0, this.history.length - 5); // only keep the latest few, no need for more. } }); } From cc30d924bf44a7c9c34593b968089f8b243f576c Mon Sep 17 00:00:00 2001 From: ccremer Date: Mon, 13 Mar 2023 14:56:00 +0100 Subject: [PATCH 3/4] Navigate back in history when saving as well --- .../billingentity-members.component.ts | 4 +++- .../organization-form/organization-form.component.ts | 8 +++++--- .../organization-members-edit.component.ts | 9 +++++---- src/app/teams/team-edit/team-edit.component.ts | 9 +++++---- 4 files changed, 18 insertions(+), 12 deletions(-) diff --git a/src/app/billingentity/billingentity-members/billingentity-members.component.ts b/src/app/billingentity/billingentity-members/billingentity-members.component.ts index 5f8078d2..35c42cf6 100644 --- a/src/app/billingentity/billingentity-members/billingentity-members.component.ts +++ b/src/app/billingentity/billingentity-members/billingentity-members.component.ts @@ -15,6 +15,7 @@ import { defaultIfNotFound } from '../../store/kubernetes-collection.service'; import { ClusterRoleCollectionService } from '../../store/cluster-role-collection.service'; import { ClusterRole } from '../../types/clusterRole'; import { KubeObject } from '../../types/entity'; +import { NavigationService } from '../../shared/navigation.service'; interface Payload { billingEntity: BillingEntity; @@ -54,6 +55,7 @@ export class BillingentityMembersComponent implements OnInit, OnDestroy { constructor( private route: ActivatedRoute, private router: Router, + private navigationService: NavigationService, private billingService: BillingEntityCollectionService, private roleService: ClusterRoleCollectionService, public rolebindingService: ClusterRolebindingCollectionService, @@ -234,7 +236,7 @@ export class BillingentityMembersComponent implements OnInit, OnDestroy { severity: 'success', summary: $localize`Successfully saved`, }); - void this.router.navigate(['../..'], { relativeTo: this.route }); + this.navigationService.back(); }, error: (error) => { this.messageService.add({ diff --git a/src/app/organizations/organization-form/organization-form.component.ts b/src/app/organizations/organization-form/organization-form.component.ts index 5a32b51d..a6ef2594 100644 --- a/src/app/organizations/organization-form/organization-form.component.ts +++ b/src/app/organizations/organization-form/organization-form.component.ts @@ -8,6 +8,7 @@ import { MessageService } from 'primeng/api'; import { OrganizationNameService } from '../organization-name.service'; import { OrganizationCollectionService } from '../../store/organization-collection.service'; import { BillingEntity } from '../../types/billing-entity'; +import { NavigationService } from '../../shared/navigation.service'; @Component({ selector: 'app-organization-form', @@ -40,7 +41,8 @@ export class OrganizationFormComponent implements OnInit, OnDestroy { private activatedRoute: ActivatedRoute, private messageService: MessageService, private organizationNameService: OrganizationNameService, - public organizationCollectionService: OrganizationCollectionService + public organizationCollectionService: OrganizationCollectionService, + private navigationService: NavigationService ) {} ngOnInit(): void { @@ -114,12 +116,11 @@ export class OrganizationFormComponent implements OnInit, OnDestroy { severity: 'success', summary: $localize`Successfully saved`, }); - void this.router.navigate(['..'], { relativeTo: this.activatedRoute }); + this.navigationService.back(); } private saveOrUpdateFailure(err: Error): void { let detail = ''; - console.debug('error!', err); if ('message' in err) { detail = err.message; } @@ -132,6 +133,7 @@ export class OrganizationFormComponent implements OnInit, OnDestroy { this.messageService.add({ severity: 'error', summary: $localize`Error`, + sticky: true, detail, }); } diff --git a/src/app/organizations/organization-members-edit/organization-members-edit.component.ts b/src/app/organizations/organization-members-edit/organization-members-edit.component.ts index 3753109f..d745bfe7 100644 --- a/src/app/organizations/organization-members-edit/organization-members-edit.component.ts +++ b/src/app/organizations/organization-members-edit/organization-members-edit.component.ts @@ -1,5 +1,5 @@ import { ChangeDetectionStrategy, Component, OnInit } from '@angular/core'; -import { ActivatedRoute, Router } from '@angular/router'; +import { ActivatedRoute } from '@angular/router'; import { faClose, faSave, faWarning } from '@fortawesome/free-solid-svg-icons'; import { OrganizationMembers } from '../../types/organization-members'; import { FormArray, FormControl, FormGroup, Validators } from '@angular/forms'; @@ -8,6 +8,7 @@ import { MessageService } from 'primeng/api'; import { RoleBinding } from 'src/app/types/role-binding'; import { OrganizationMembersCollectionService } from '../../store/organizationmembers-collection.service'; import { RolebindingCollectionService } from '../../store/rolebinding-collection.service'; +import { NavigationService } from '../../shared/navigation.service'; interface Payload { members: OrganizationMembers; @@ -45,9 +46,9 @@ export class OrganizationMembersEditComponent implements OnInit { constructor( private activatedRoute: ActivatedRoute, private messageService: MessageService, - private router: Router, private membersService: OrganizationMembersCollectionService, - private rolebindingService: RolebindingCollectionService + private rolebindingService: RolebindingCollectionService, + private navigationService: NavigationService ) {} get userRefs(): FormArray | undefined { @@ -165,7 +166,7 @@ export class OrganizationMembersEditComponent implements OnInit { severity: 'success', summary: $localize`Successfully saved`, }); - void this.router.navigate(['../..'], { relativeTo: this.activatedRoute }); + this.navigationService.back(); }, error: (error) => { this.messageService.add({ diff --git a/src/app/teams/team-edit/team-edit.component.ts b/src/app/teams/team-edit/team-edit.component.ts index dd7d036a..ec3f2791 100644 --- a/src/app/teams/team-edit/team-edit.component.ts +++ b/src/app/teams/team-edit/team-edit.component.ts @@ -1,11 +1,12 @@ import { ChangeDetectionStrategy, Component, OnInit } from '@angular/core'; import { faClose, faSave, faWarning } from '@fortawesome/free-solid-svg-icons'; import { Team } from '../../types/team'; -import { ActivatedRoute, Router } from '@angular/router'; +import { ActivatedRoute } from '@angular/router'; import { FormArray, FormBuilder, FormControl, FormGroup, Validators } from '@angular/forms'; import { MessageService } from 'primeng/api'; import { Observable, of, take, tap } from 'rxjs'; import { TeamCollectionService } from '../../store/team-collection.service'; +import { NavigationService } from '../../shared/navigation.service'; @Component({ selector: 'app-team-edit', @@ -23,10 +24,10 @@ export class TeamEditComponent implements OnInit { constructor( private formBuilder: FormBuilder, - private router: Router, private activatedRoute: ActivatedRoute, private messageService: MessageService, - public teamService: TeamCollectionService + public teamService: TeamCollectionService, + private navigationService: NavigationService ) {} get userRefs(): FormArray { @@ -71,7 +72,7 @@ export class TeamEditComponent implements OnInit { severity: 'success', summary: $localize`Successfully saved`, }); - void this.router.navigate(['../..'], { relativeTo: this.activatedRoute }); + this.navigationService.back(); }, error: (error) => { let detail = ''; From 46c70c6f1d6a54b6d2239044e27d93e58d854db0 Mon Sep 17 00:00:00 2001 From: ccremer Date: Wed, 15 Mar 2023 10:02:44 +0100 Subject: [PATCH 4/4] Extend directive to accept a default path value if not enough history --- .../billingentity-members.component.html | 2 +- .../billingentity-members.component.ts | 2 +- .../billingentity-view.component.html | 2 +- .../organization-edit.component.html | 2 +- .../organization-form.component.ts | 2 +- .../organization-members-edit.component.html | 2 +- .../organization-members-edit.component.ts | 5 +++-- src/app/shared/back-link.directive.ts | 15 ++++++++++--- src/app/shared/navigation.service.ts | 22 +++++++++++-------- .../teams/team-edit/team-edit.component.html | 2 +- .../teams/team-edit/team-edit.component.ts | 5 +++-- src/app/zones/zone/zone-detail.component.html | 2 +- 12 files changed, 39 insertions(+), 24 deletions(-) diff --git a/src/app/billingentity/billingentity-members/billingentity-members.component.html b/src/app/billingentity/billingentity-members/billingentity-members.component.html index c27be7b0..71b80cc2 100644 --- a/src/app/billingentity/billingentity-members/billingentity-members.component.html +++ b/src/app/billingentity/billingentity-members/billingentity-members.component.html @@ -6,7 +6,7 @@ {{ payload.billingEntity.metadata.name }} Members - + diff --git a/src/app/billingentity/billingentity-members/billingentity-members.component.ts b/src/app/billingentity/billingentity-members/billingentity-members.component.ts index 35c42cf6..02015dda 100644 --- a/src/app/billingentity/billingentity-members/billingentity-members.component.ts +++ b/src/app/billingentity/billingentity-members/billingentity-members.component.ts @@ -236,7 +236,7 @@ export class BillingentityMembersComponent implements OnInit, OnDestroy { severity: 'success', summary: $localize`Successfully saved`, }); - this.navigationService.back(); + void this.router.navigate([this.navigationService.previousLocation()], { relativeTo: this.route }); }, error: (error) => { this.messageService.add({ diff --git a/src/app/billingentity/billingentity-view/billingentity-view.component.html b/src/app/billingentity/billingentity-view/billingentity-view.component.html index fb276147..8770e9a8 100644 --- a/src/app/billingentity/billingentity-view/billingentity-view.component.html +++ b/src/app/billingentity/billingentity-view/billingentity-view.component.html @@ -4,7 +4,7 @@
{{ billingEntity.metadata.name }}
- +
diff --git a/src/app/organizations/organization-edit/organization-edit.component.html b/src/app/organizations/organization-edit/organization-edit.component.html index b3518777..a604bdc0 100644 --- a/src/app/organizations/organization-edit/organization-edit.component.html +++ b/src/app/organizations/organization-edit/organization-edit.component.html @@ -8,7 +8,7 @@ {{ payload.organization.metadata.name }}
- + diff --git a/src/app/organizations/organization-form/organization-form.component.ts b/src/app/organizations/organization-form/organization-form.component.ts index a6ef2594..511a5aed 100644 --- a/src/app/organizations/organization-form/organization-form.component.ts +++ b/src/app/organizations/organization-form/organization-form.component.ts @@ -116,7 +116,7 @@ export class OrganizationFormComponent implements OnInit, OnDestroy { severity: 'success', summary: $localize`Successfully saved`, }); - this.navigationService.back(); + void this.router.navigate([this.navigationService.previousLocation()], { relativeTo: this.activatedRoute }); } private saveOrUpdateFailure(err: Error): void { diff --git a/src/app/organizations/organization-members-edit/organization-members-edit.component.html b/src/app/organizations/organization-members-edit/organization-members-edit.component.html index 29b24160..0a7dae3f 100644 --- a/src/app/organizations/organization-members-edit/organization-members-edit.component.html +++ b/src/app/organizations/organization-members-edit/organization-members-edit.component.html @@ -6,7 +6,7 @@ {{ payload.members.metadata.namespace }} Members - + diff --git a/src/app/organizations/organization-members-edit/organization-members-edit.component.ts b/src/app/organizations/organization-members-edit/organization-members-edit.component.ts index d745bfe7..92718e9a 100644 --- a/src/app/organizations/organization-members-edit/organization-members-edit.component.ts +++ b/src/app/organizations/organization-members-edit/organization-members-edit.component.ts @@ -1,5 +1,5 @@ import { ChangeDetectionStrategy, Component, OnInit } from '@angular/core'; -import { ActivatedRoute } from '@angular/router'; +import { ActivatedRoute, Router } from '@angular/router'; import { faClose, faSave, faWarning } from '@fortawesome/free-solid-svg-icons'; import { OrganizationMembers } from '../../types/organization-members'; import { FormArray, FormControl, FormGroup, Validators } from '@angular/forms'; @@ -46,6 +46,7 @@ export class OrganizationMembersEditComponent implements OnInit { constructor( private activatedRoute: ActivatedRoute, private messageService: MessageService, + private router: Router, private membersService: OrganizationMembersCollectionService, private rolebindingService: RolebindingCollectionService, private navigationService: NavigationService @@ -166,7 +167,7 @@ export class OrganizationMembersEditComponent implements OnInit { severity: 'success', summary: $localize`Successfully saved`, }); - this.navigationService.back(); + void this.router.navigate([this.navigationService.previousLocation()], { relativeTo: this.activatedRoute }); }, error: (error) => { this.messageService.add({ diff --git a/src/app/shared/back-link.directive.ts b/src/app/shared/back-link.directive.ts index a1f1b89a..b85afe17 100644 --- a/src/app/shared/back-link.directive.ts +++ b/src/app/shared/back-link.directive.ts @@ -1,14 +1,23 @@ -import { Directive, HostListener } from '@angular/core'; +import { Directive, HostListener, Input } from '@angular/core'; import { NavigationService } from './navigation.service'; +import { ActivatedRoute, Router } from '@angular/router'; +/** + * This directive adds a `click` event listener to navigate back in history. + * It accepts an input that is used as the default path in case there is no history (e.g. opened link in a new tab). + */ @Directive({ selector: '[appBackLink]', }) export class BackLinkDirective { - constructor(private navigation: NavigationService) {} + constructor(private navigation: NavigationService, private router: Router, private activatedRoute: ActivatedRoute) {} + + @Input() + appBackLink?: string; @HostListener('click') onClick(): void { - this.navigation.back(); + const route = this.navigation.previousLocation(this.appBackLink); + void this.router.navigate([route], { relativeTo: this.activatedRoute }); } } diff --git a/src/app/shared/navigation.service.ts b/src/app/shared/navigation.service.ts index 41be674c..f3230244 100644 --- a/src/app/shared/navigation.service.ts +++ b/src/app/shared/navigation.service.ts @@ -1,15 +1,15 @@ import { Injectable } from '@angular/core'; -import { Location } from '@angular/common'; import { NavigationEnd, Router } from '@angular/router'; /** - * See https://nils-mehlhorn.de/posts/angular-navigate-back-previous-page/ + * Inspired by https://nils-mehlhorn.de/posts/angular-navigate-back-previous-page/ + * Modified to be used with a Router working with default and relative paths in case the history is empty. */ @Injectable({ providedIn: 'root' }) export class NavigationService { private history: string[] = []; - constructor(private router: Router, private location: Location) { + constructor(private router: Router) { this.router.events.subscribe((event) => { if (event instanceof NavigationEnd) { this.history.push(event.urlAfterRedirects); @@ -18,14 +18,18 @@ export class NavigationService { }); } - back(): void { - const record = this.history.pop(); + /** + * Gets the previous URI location in the history. + * @param defaultPath if the history is empty, return this path as fallback value + * @returns the URI, or '/' if no default was given. + */ + previousLocation(defaultPath?: string): string { + void this.history.pop(); // remove "current" location if (this.history.length > 0) { - this.location.back(); + const previousLocation = this.history.pop(); + return previousLocation ?? defaultPath ?? '/'; } else { - window.history.replaceState(null, '', '/'); - window.history.pushState(null, '', record ?? this.router.url); - this.location.back(); + return defaultPath ?? '/'; } } } diff --git a/src/app/teams/team-edit/team-edit.component.html b/src/app/teams/team-edit/team-edit.component.html index 8f594aed..0c1f1ead 100644 --- a/src/app/teams/team-edit/team-edit.component.html +++ b/src/app/teams/team-edit/team-edit.component.html @@ -9,7 +9,7 @@ {{ team.metadata.name }} - + diff --git a/src/app/teams/team-edit/team-edit.component.ts b/src/app/teams/team-edit/team-edit.component.ts index ec3f2791..067f685f 100644 --- a/src/app/teams/team-edit/team-edit.component.ts +++ b/src/app/teams/team-edit/team-edit.component.ts @@ -1,7 +1,7 @@ import { ChangeDetectionStrategy, Component, OnInit } from '@angular/core'; import { faClose, faSave, faWarning } from '@fortawesome/free-solid-svg-icons'; import { Team } from '../../types/team'; -import { ActivatedRoute } from '@angular/router'; +import { ActivatedRoute, Router } from '@angular/router'; import { FormArray, FormBuilder, FormControl, FormGroup, Validators } from '@angular/forms'; import { MessageService } from 'primeng/api'; import { Observable, of, take, tap } from 'rxjs'; @@ -24,6 +24,7 @@ export class TeamEditComponent implements OnInit { constructor( private formBuilder: FormBuilder, + private router: Router, private activatedRoute: ActivatedRoute, private messageService: MessageService, public teamService: TeamCollectionService, @@ -72,7 +73,7 @@ export class TeamEditComponent implements OnInit { severity: 'success', summary: $localize`Successfully saved`, }); - this.navigationService.back(); + void this.router.navigate([this.navigationService.previousLocation()], { relativeTo: this.activatedRoute }); }, error: (error) => { let detail = ''; diff --git a/src/app/zones/zone/zone-detail.component.html b/src/app/zones/zone/zone-detail.component.html index 41b100ef..d08c3ff3 100644 --- a/src/app/zones/zone/zone-detail.component.html +++ b/src/app/zones/zone/zone-detail.component.html @@ -41,7 +41,7 @@