Ability to Pin Views

[hriday-panchasara]

• Add Ability to pin views to the top of the layout so that they stay visible when scrolling
• Current implementation is to allow pinning only one view at a time
• View is scrolled into focus when pinned/unpinned
• Pinned view persists when page refreshed
• Added additional resize handlers to pinned view in case height exceeds page length

[hriday-panchasara]

Our current layout is using two containers - one for timeline-charts and below it one for non-timeline-charts. In order to show a pinned view I've added another container on top of these two, so that any view when pinned would move to this top container without breaking the existing layout. The only issue is that when we re-render trace-context-component to show this new layout, the new Virtual DOM tree shows that an output is added to / removed from the pinned views container, hence re-rendering the output-view that just got pinned/unpinned. Since we don't keep track of the of the output component's tree, we lose the selections that are made. A future improvement could be to persist the state of the abstract-tree-output component

[bhufmann]

Nice feature!

[bhufmann]

Our current layout is using two containers - one for timeline-charts and below it one for non-timeline-charts. In order to show a pinned view I've added another container on top of these two, so that any view when pinned would move to this top container without breaking the existing layout. The only issue is that when we re-render trace-context-component to show this new layout, the new Virtual DOM tree shows that an output is added to / removed from the pinned views container, hence re-rendering the output-view that just got pinned/unpinned. Since we don't keep track of the of the output component's tree, we lose the selections that are made. A future improvement could be to persist the state of the abstract-tree-output component

I think it would be beneficial to persist the state of view that's being pinned and unpinned. When running this change, I expected that the view is just re-arranged to the top, but nothing else changes. Having to redo the selections in the tree or scrolling vertically doesn't provide a good UX. It should just behave like moving the output component manually with the left mouse button drag.

@hriday-panchasara what to you suggest as next steps to also achieve this?

[bhufmann]

Another question: Should we add a visual indicator that the view is pinned? Maybe add a pin or something similar somewhere which is only shown when it's pinned.

[hriday-panchasara]

Thanks @bhufmann , unfortunately I'm not sure how to pin/unpin views without losing track of state. I tried experimenting with react-grid-layout, but the only way I found to keep a view visible while scrolling was to put it in a separate container and make that container static through CSS. For non-timescale layouts the state can be persisted as datatree-output-component is static and events-table reflects the selected events in the timerange. We can think of a future enhancement to have an option to keep track of abstract-tree-output-component's state on rerender. Since we're only providing the ability to pin one view at the time, and it can only be accessed by the options menu, I'm assuming that toggling pin/unpin won't be used excessively and losing state is a small drawback that we can consider.

[bhufmann]

Thanks @bhufmann , unfortunately I'm not sure how to pin/unpin views without losing track of state. I tried experimenting with react-grid-layout, but the only way I found to keep a view visible while scrolling was to put it in a separate container and make that container static through CSS. For non-timescale layouts the state can be persisted as datatree-output-component is static and events-table reflects the selected events in the timerange. We can think of a future enhancement to have an option to keep track of abstract-tree-output-component's state on rerender. Since we're only providing the ability to pin one view at the time, and it can only be accessed by the options menu, I'm assuming that toggling pin/unpin won't be used excessively and losing state is a small drawback that we can consider.

I think loosing the state would not be a good UX. However, I'm ok that we work on merging this PR and have a separate PR for keeping the state.

[bhufmann]

I tested it and it works well. The location of the pin feels a bit out of place (see picture below). It should be aligned next to the hamburger menu. It should also show some tooltip to indicate what it means when users hover over it with the mouse. Could we even make it a button to unpin?

Secondly, when pinning the events table it is shown under time axis. Could we move it and other non-time-axis views above the axis instead when pinning? Time-axis views (timegraph or XY) would stay below the time axis.

[bhufmann]

I just noticed that the following error was in the console log. Not exactly sure when it happened (maybe during unpin):

root ERROR Warning: Can't perform a React state update on an unmounted component. This is a no-op, but it indicates a memory leak in your application. To fix, cancel all subscriptions and asynchronous tasks in the componentWillUnmount method.
    in TableOutputComponent (created by TraceContextComponent)
    in Resizable (created by GridItem)
    in DraggableCore (created by GridItem)
    in GridItem (created by ReactGridLayout)
    in div (created by ReactGridLayout)
    in ReactGridLayout (created by ResponsiveReactGridLayout)
    in ResponsiveReactGridLayout (created by WidthProvider)
    in WidthProvider (created by TraceContextComponent)

Have you seen that?

[hriday-panchasara]

I tested it and it works well. The location of the pin feels a bit out of place (see picture below). It should be aligned next to the hamburger menu. It should also show some tooltip to indicate what it means when users hover over it with the mouse. Could we even make it a button to unpin?

Secondly, when pinning the events table it is shown under time axis. Could we move it and other non-time-axis views above the axis instead when pinning? Time-axis views (timegraph or XY) would stay below the time axis.

I've added a tooltip to the icon on hover, but I'm not sure if its a good idea to have the icon act as a button. We decided to use a hamburger menu because we want charts to have multiple options in the future (filter, export, pin), and we want the menu to be shown on overflow as well. The below picture shows an example where the pin icon is not shown because of title bar label overflow

Like you mentioned, we could align the pin icon next to the hamburger menu, but then we'll have to increase the output view's min height to keep these buttons visible. The following picture shows the current layout where the view's min height is set to show both the hamburger menu and the close icon:

Regarding having the icon act as a button, my initial idea was to do the same, but while testing it out at times doing drag and drop for views I clicked the button by mistake, which made me think of it as inconvenient UX.

I've adjusted the layout to show pinned non-time-axis views above the time-axis:

[hriday-panchasara]

I just noticed that the following error was in the console log. Not exactly sure when it happened (maybe during unpin):

root ERROR Warning: Can't perform a React state update on an unmounted component. This is a no-op, but it indicates a memory leak in your application. To fix, cancel all subscriptions and asynchronous tasks in the componentWillUnmount method.
    in TableOutputComponent (created by TraceContextComponent)
    in Resizable (created by GridItem)
    in DraggableCore (created by GridItem)
    in GridItem (created by ReactGridLayout)
    in div (created by ReactGridLayout)
    in ReactGridLayout (created by ResponsiveReactGridLayout)
    in ResponsiveReactGridLayout (created by WidthProvider)
    in WidthProvider (created by TraceContextComponent)

Have you seen that?

Yeah, I see it only when performing pin/unpin on the events table. Fixed the issue

[bhufmann]

Looks good to me. Thanks!