ng: functional settings dialog #2794
Conversation
Note that this does not have the user experience we ultimately want. This change only contains functional improvements.
tensorboard/components/tf_ng_tensorboard/settings/dialog.component.ts
Outdated
Show resolved
Hide resolved
| selector: 'settings-button', | ||
| template: ` | ||
| <button mat-button (click)="openDialog()"> | ||
| <mat-icon>more_vert</mat-icon> |
There was a problem hiding this comment.
Locally, I can't get this icon to appear. Are we including material icons fonts somewhere?
[1] makes it sound like "You will still need to include the HTML to load the font"
There was a problem hiding this comment.
I opted out of using icon fonts but instead use the svg variant of mat-icon. I've not done the set up all correctly just yet. I was going to revisit this later.
There was a problem hiding this comment.
Ack. Maybe add a TODO somewhere? Up to you
There was a problem hiding this comment.
Because how it looks is self-evident of its TODO-ness, I will omit a code TODO.
tensorboard/components/tf_ng_tensorboard/settings/settings.component.ts
Outdated
Show resolved
Hide resolved
| name = "settings", | ||
| srcs = [ | ||
| "dialog.component.ts", | ||
| "settings.component.ts", |
There was a problem hiding this comment.
"settings.component.ts" > "settingsUI.component.ts" or "button.component.ts" ?
There was a problem hiding this comment.
Changed to settings_button.component.ts. This is an "entry point" from other component/module's point of view so I decided to not use very generic name like button.component.ts (kinda like how material components are prefixed with Mat)
| MatCheckboxModule, | ||
| MatDialogModule, | ||
| MatIconModule, | ||
| MatInputModule, |
There was a problem hiding this comment.
Is there a way for me to know:
- Which Component uses a specific module?
- Which Directives from module X are used?
I can't easily check for unused imported modules without understanding the context of each one. Just curious whether that's the intended Angular approach
There was a problem hiding this comment.
AFAIK, nope. I can imagine writing tool that checks for strict immediate deps and have such check but we do not have one by default.
| private readonly reloadPeriodInSec$ = this.store.pipe( | ||
| select(getReloadPeriodInSec) | ||
| ); | ||
| readonly reloadPeriodControl = new FormControl(15, [ |
There was a problem hiding this comment.
Should the default value be the store's value, instead of '15'? Or 'null', if it doesn't matter?
There was a problem hiding this comment.
view should not know anything about store. null sounds good but it would fail the condition on initial render so I made it the min value.
There was a problem hiding this comment.
Stepping via debugger, it seems like the initial render takes the initial value from the store (ngOnInit and the setValue fire before the first render). Either sounds fine, though
Note that this does not have the user experience we ultimately want.
This change only contains functional improvements.