core: extend page title module functionalities #5121
Conversation
Extend PageTitleModule to be used by TensorBoard.corp and
TensorBoard.dev surfaces with the following additional support:
- Allows service specific suffix in page title, e.g. `TensorBoard.corp`.
- Uses non-empty experiment name to name the single experiment
dashboard page.
| return experimentName; | ||
| } else { | ||
| return this.tbServiceName | ||
| ? `TensorBoard.${this.tbServiceName}` |
There was a problem hiding this comment.
Instead of making the service name only the part after "TensorBoard.", could we make it the entire string? As a hypothetical example, something like "Vertex TensorBoard" might be desired one day, which does not begin with the "TensorBoard." prefix.
There was a problem hiding this comment.
+1 - also, for consistency maybe nice to refer to this as the "brand" (like tbBrandName) since that's the term we've used for the header logo text, historically:
| return experimentName; | ||
| } else { | ||
| return this.tbServiceName | ||
| ? `TensorBoard.${this.tbServiceName}` |
There was a problem hiding this comment.
+1 - also, for consistency maybe nice to refer to this as the "brand" (like tbBrandName) since that's the term we've used for the header logo text, historically:
| map(([env, experimentName]) => { | ||
| const tbBrandName = this.customBrandName | ||
| ? this.customBrandName | ||
| : 'TensorBoard'; |
There was a problem hiding this comment.
Since we're using this hardcoded string again on L93, could we extract it into something like const DEFAULT_BRAND_NAME = 'TensorBoard' near the top of the file?
optional: this.customBrandName || DEFAULT_BRAND_NAME also works and is shorter, if you prefer
| constructor(private readonly store: Store<State>) {} | ||
| constructor( | ||
| private readonly store: Store<State>, | ||
| @Optional() @Inject(TB_BRAND_NAME) readonly customBrandName: string |
There was a problem hiding this comment.
nit: is it possible to make private readonly?
| ); | ||
| }); | ||
|
|
||
| it('uses `Tensorboard` as default tab title', () => { |
There was a problem hiding this comment.
Also, could we also add a test case covering the comparison route, specifically? Right now it just shows the brand name, but might be worth considering something fancier later.
There was a problem hiding this comment.
SG, added a test case :)
also rename the `tab title` to `page title` for consistency
Extend PageTitleModule to be used by TensorBoard.corp and TensorBoard.dev surfaces.
Uses
TensorBoardas default page/tab title;If specified (and non-empty), flag
window_titlewill overwrite page title in all cases;If specified (and non-empty), uses the given Tensorboard brand name as page title, e.g.
TensorBoard.corp;Uses non-empty experiment name to name the single experiment
${experimentName} - ${tensorboardBrandName}, example:For Googlers, please refer to issue b/189246978.