core: make sidebar resizable #5219
Conversation
This change introduces new layout component that you can use to have consistent layout in a plugin. This layout component is responsible for interactions and remembering the last set width.
| import {MouseEventButtons} from '../../util/dom'; | ||
|
|
||
| @Component({ | ||
| selector: 'tb-layout', |
There was a problem hiding this comment.
Consider including "resizable" in the name, e.g. "tb-resizable-layout", for more descriptiveness.
There was a problem hiding this comment.
I am iffy about that. I want this to be used in all pages' layout when possible like https://github.com/tensorflow/tensorboard/blob/master/tensorboard/components/tf_dashboard_common/tf-dashboard-layout.ts#L23 and whether it is resizable or not is not the part I want plugin authors to care/know.
There was a problem hiding this comment.
Hm, then perhaps something along the lines of tb-dashboard-layout or tb-plugin-dashboard-layout? With tb-layout, it's unclear to me when it should be used.
| return createSelector<State, number, PersistableSettings>( | ||
| getSideBarWidthInPercent, | ||
| (sideBarWidthInPercent) => { | ||
| return {sideBarWidthInPercent}; |
There was a problem hiding this comment.
If we plan to ever have another resizable layout in the UI later on, which also needs its width persisted, it would be reasonable to separately store their widths. To prevent future ambiguity as to which sidebar this setting refers to, would it reasonable to name this property something specific like metricsRunSidebarWidth?
There was a problem hiding this comment.
I think my comment below makes this obsolete.
| @@ -37,6 +37,8 @@ export interface CoreState { | |||
| // For now, we want them here for Polymer interop states reasons, too. | |||
| polymerInteropRuns: Run[]; | |||
| polymerInteropRunSelection: Set<RunId>; | |||
| // Number between 0 and 100. | |||
| sideBarWidthInPercent: number; | |||
There was a problem hiding this comment.
I originally expected to see the value stored under metrics/, since it is specific to Time Series UI. Having it under core/ makes sense when under the assumption that there will not be other resizable layouts. Would it be better to move this field, the reducer, selector, etc to metrics/ as "(get)metricsRunSidebarWidth", if we want other features besides metrics/ to have persisted sidebar widths? e.g. maybe the Hparams dashboard remembers its own sidebar width separately in the future.
If not, could we at least add comments to here and in the selector definition that mention it is specific to Time Series' run sidebar?
There was a problem hiding this comment.
This change introduces new layout component that you can use to have
consistent layout in a plugin.
This component is expected to be used by other plugins as the description indicates.
For instance, I want this to be usable in npmi plugin which I have in a follow up change. I think it makes sense for all sidebar widths to be linked thus this state remains in the Core plugin.
There was a problem hiding this comment.
Thanks for clarifying, I see where this is going now. This setting and the tb-layout component isn't to be used to make any resizable sidebar, (e.g. not to be used for cases like the Time Series settings pane), but specifically only for replacing the top-level layout for each plugin dashboard.
The opposite was what I was looking for. I wanted the width of the sidebar to be relative to the size of the window which often is what I am looking for in a UI. |
| import {MouseEventButtons} from '../../util/dom'; | ||
|
|
||
| @Component({ | ||
| selector: 'tb-layout', |
There was a problem hiding this comment.
Hm, then perhaps something along the lines of tb-dashboard-layout or tb-plugin-dashboard-layout? With tb-layout, it's unclear to me when it should be used.
| @@ -37,6 +37,8 @@ export interface CoreState { | |||
| // For now, we want them here for Polymer interop states reasons, too. | |||
| polymerInteropRuns: Run[]; | |||
| polymerInteropRunSelection: Set<RunId>; | |||
| // Number between 0 and 100. | |||
| sideBarWidthInPercent: number; | |||
There was a problem hiding this comment.
Thanks for clarifying, I see where this is going now. This setting and the tb-layout component isn't to be used to make any resizable sidebar, (e.g. not to be used for cases like the Time Series settings pane), but specifically only for replacing the top-level layout for each plugin dashboard.
This change introduces new layout component that you can use to have consistent layout in a plugin. This layout component is responsible for interactions and remembering the last set width.
This change introduces new layout component that you can use to have consistent layout in a plugin. This layout component is responsible for interactions and remembering the last set width.
This change introduces new layout component that you can use to have
consistent layout in a plugin. This layout component is responsible for
interactions and remembering the last set width.