You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@trafficcontrol.apache.org by GitBox <gi...@apache.org> on 2022/06/22 17:20:30 UTC
[GitHub] [trafficcontrol] ocket8888 commented on a diff in pull request #6914: Add Ability to Extend TPv2
ocket8888 commented on code in PR #6914:
URL: https://github.com/apache/trafficcontrol/pull/6914#discussion_r904016191
##########
experimental/traffic-portal/src/app/app-routing.module.ts:
##########
@@ -22,22 +24,37 @@ const routes: Routes = [
{
canLoad: [AuthenticatedGuard],
children: [{
- loadChildren: async (): Promise<object> => import("./core/core.module")
- .then(mod => mod.CoreModule),
+ loadChildren: async (): Promise<Type<unknown>> => {
+ const imp = import("./core/core.module");
+ return imp.then(mod => mod.CoreModule) as Promise<Type<typeof imp>>;
+ },
Review Comment:
I think the reason you made this a multi-line function block is because you needed to do the `as` statement to make the compiler happy. But you actually don't need to do that. Just change the return type from `object` to `Type<CoreModule>` (you'll need to import the `CoreModule` type). Same applies below to the `CustomModule` lazy load.
##########
experimental/traffic-portal/src/app/core/cache-groups/cache-group-table/cache-group-table.component.ts:
##########
@@ -132,7 +132,7 @@ export class CacheGroupTableComponent implements OnInit {
public fuzzySubject: BehaviorSubject<string>;
/** Form controller for the user search input. */
- public fuzzControl: FormControl = new FormControl("");
+ public fuzzControl: UntypedFormControl = new UntypedFormControl("");
Review Comment:
This is fine for now since this PR is big enough as-is, but we should make an issue to make these things typed. (Although honestly we're never using any actual FormControl features so most of these could probably use `ngModel` bindings instead).
##########
experimental/traffic-portal/src/app/app-routing.module.ts:
##########
@@ -11,8 +11,10 @@
* See the License for the specific language governing permissions and
* limitations under the License.
*/
-import { NgModule } from "@angular/core";
-import { RouterModule, Routes } from "@angular/router";
+import {NgModule, Type} from "@angular/core";
+import {RouterModule, Routes} from "@angular/router";
Review Comment:
nit on line 14: `Type` is only used as a type, could be a `type` import
line 15: doesn't appear to have any content changes, could just be left alone
##########
experimental/traffic-portal/src/app/shared/tp-header/tp-header.service.spec.ts:
##########
@@ -15,7 +15,7 @@ import { TestBed } from "@angular/core/testing";
import {TpHeaderComponent} from "src/app/shared/tp-header/tp-header.component";
-import { TpHeaderService } from "./tp-header.service";
+import {HeaderNavigation, TpHeaderService} from "./tp-header.service";
Review Comment:
nit: `HeaderNavigation` is only used as a type, could be a `type` import
although if you take my earlier advice and have the service own its default links this import will be unnecessary anyway.
##########
experimental/traffic-portal/src/app/shared/generic-table/generic-table.component.html:
##########
@@ -21,7 +21,7 @@
<fa-icon [icon]="caretIcon" class="caret" [ngClass]="{'rotate': showMenu}"></fa-icon>
</button>
<mat-menu #menu="matMenu">
- <button mat-menu-item *ngFor="let c of columns; orderBy:'colDef.headerName'" (click)="toggleVisibility($event, c.getColId())">
+ <button mat-menu-item *ngFor="let c of columns" (click)="toggleVisibility($event, c.getColId())">
Review Comment:
I think the problem here is just that it's trying to order by `colDef.headerName` but the loop variable is `c`. Idk why that's compiling, frankly.
##########
experimental/traffic-portal/src/app/shared/tp-header/tp-header.component.html:
##########
@@ -19,30 +19,36 @@ <h1>{{title ? title : 'Welcome to Traffic Portal!'}}</h1>
<div></div>
<nav id="expanded">
<ul>
- <li><a mat-button routerLink="/core/">Home</a></li>
- <li *ngIf="hasPermission('USER:READ')"><a mat-button routerLink="/core/users">Users</a></li>
- <li *ngIf="hasPermission('SERVER:READ')"><a mat-button routerLink="/core/servers">Servers</a></li>
+ <li *ngFor="let nav of horizNavs">
+ <a *ngIf="navShown(nav, 'anchor')" mat-button [routerLink]="nav.routerLink">{{nav.text}}</a>
+ <button *ngIf="navShown(nav, 'button')" mat-button (click)="nav?.click()">{{nav.text}}</button>
+ </li>
<li>
<button mat-icon-button [matMenuTriggerFor]="expandedMenu">
<mat-icon>manage_accounts</mat-icon>
</button>
<mat-menu #expandedMenu="matMenu">
- <a mat-menu-item routerLink="/core/me">Profile</a>
- <button mat-menu-item (click)="logout()">Logout</button>
<button mat-menu-item [matMenuTriggerFor]="themeMenu">Theme</button>
+ <div *ngFor="let nav of vertNavs">
+ <a *ngIf="navShown(nav, 'anchor')" mat-menu-item [routerLink]="nav.routerLink">{{nav.text}}</a>
+ <button *ngIf="navShown(nav, 'button')" mat-menu-item (click)="nav?.click()">{{nav.text}}</button>
Review Comment:
I'm pretty sure there's a way to do the typing so that button nav items aren't allowed to have a null/undefined `click` property. It seems really similar to what I did for the generic table's context menu items.
##########
experimental/traffic-portal/src/app/shared/tp-header/tp-header.component.ts:
##########
@@ -37,17 +37,55 @@ export class TpHeaderComponent implements OnInit {
public hidden = false;
+ // Will try to display each of these navs on the header, space allowing.
+ public horizNavs: Array<HeaderNavigation> = new Array<HeaderNavigation>();
+ // Navs that are not directly displayed on the header.
+ public vertNavs: Array<HeaderNavigation> = new Array<HeaderNavigation>();
+
/**
* Angular lifecycle hook
*/
public ngOnInit(): void {
+ this.headerSvc.addHorizontalNav({
+ routerLink: "/core",
+ text: "Home",
+ type: "anchor",
+ }, "home");
+ this.headerSvc.addHorizontalNav({
+ routerLink: "/core/users",
+ text: "Users",
+ type: "anchor",
+ visible: () => this.hasPermission("USER:READ"),
+ }, "Users");
+ this.headerSvc.addHorizontalNav({
+ routerLink: "/core/servers",
+ text: "Servers",
+ type: "anchor",
+ visible: () => this.hasPermission("SERVER:READ"),
+ }, "Servers");
+ this.headerSvc.addVerticalNav({
+ routerLink: "/core/me",
+ text: "Profile",
+ type: "anchor"
+ }, "Profile");
+ this.headerSvc.addVerticalNav({
+ click: async () => this.logout(),
+ text: "Logout",
+ type: "anchor"
+ }, "Logout");
+
this.headerSvc.headerTitle.subscribe(title => {
this.title = title;
});
-
this.headerSvc.headerHidden.subscribe(hidden => {
this.hidden = hidden;
});
+ this.headerSvc.horizontalNavsUpdated.subscribe(navs => {
+ this.horizNavs = navs;
+ });
+ this.headerSvc.verticalNavsUpdated.subscribe(navs => {
+ this.vertNavs = navs;
+ });
Review Comment:
Instead of having the component attempt to re-insert these default links every time it renders - which will overwrite my overrides of default links - why not just have the service own them, which is a singleton by definition and won't overrwrite changes made through the service's API?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org