Project

General

Profile

Actions

Feature #18128

closed

Basic multi-panel view for process panel

Added by Peter Amstutz 10 months ago. Updated 7 months ago.

Status:
Resolved
Priority:
Normal
Assigned To:
Category:
Workbench2
Target version:
Start date:
10/08/2021
Due date:
% Done:

100%

Estimated time:
(Total: 0.00 h)
Story points:
-
Release relationship:
Auto

Files

Multi Panel View demo.mov (4.24 MB) Multi Panel View demo.mov Lucas Di Pentima, 10/08/2021 08:01 PM
multi-panel-view-demo2.mov (9.73 MB) multi-panel-view-demo2.mov Lucas Di Pentima, 10/19/2021 08:59 PM
Panel navigation demo.mov (12.4 MB) Panel navigation demo.mov Lucas Di Pentima, 11/18/2021 12:35 PM

Subtasks 1 (0 open1 closed)

Task #18151: Review 18128-multi-panel-viewResolvedPeter Amstutz10/08/2021

Actions

Related issues

Related to Arvados Epics - Story #16945: WB2 Workflows / containers feature parityIn Progress08/01/202107/31/2022

Actions
Actions #1

Updated by Peter Amstutz 10 months ago

  • Assigned To set to Lucas Di Pentima
Actions #2

Updated by Lucas Di Pentima 10 months ago

  • Status changed from New to In Progress
Actions #3

Updated by Peter Amstutz 9 months ago

  • Target version changed from 2021-09-29 sprint to 2021-10-13 sprint
Actions #4

Updated by Lucas Di Pentima 9 months ago

Updates at arvados-workbench2|826a036 - branch 18128-multi-panel-view
Test run: developer-tests-workbench2: #498

  • Unrelated changes: Removes unused code and forces to use https when getting the cluster config (this was biting us when running a local build of the app)
  • Adds MPVContainer and MPVPanelContent components (better naming ideas are welcome) for the "Multi-Panel View" feature:
    • MPVContainer aims to be a drop-in replacement for <Grid container ...> with the added features of handling children visibility via a button tray, and also pass new props to its children so they can optionally support this new feature.
    • MPVPanelContent aims to be a drop-in replacement for <Grid item ...> in case there's a need for a particular layout. It also forwards the new props to its children.
  • Adjusted the process info card and data explorer to handle the new optional props so that both panels inside the process view offer a "Close panel" button.

Tests are pending; I want first to validate if this approach makes sense.

If we want to "maximize" panels (via a new action/button on individual panels), we could use a similar mechanism. Theming of the button tray will also need adjusting, of course.

Actions #5

Updated by Lucas Di Pentima 9 months ago

Video demo attached

Actions #6

Updated by Peter Amstutz 9 months ago

A few thoughts:

  • As you said, the button tray needs styling
  • The panels don't have a title, so knowing which panel corresponds to which button is not completely obvious
  • Panel sizing is going to be tricky. We want to default 1/2 + 1/2 or 1/3 + 1/3 + 1/3 sizing to use all the space and avoid scrolling but individual panels will also have minimum and maximum sizes based on their content.
  • To give a better example would be nice to add a few more panels, like "inputs" and "command" currently only shown in popup dialogs.
Actions #7

Updated by Lucas Di Pentima 9 months ago

  • Target version changed from 2021-10-13 sprint to 2021-10-27 sprint
Actions #8

Updated by Lucas Di Pentima 9 months ago

Updates at arvados-workbench2|0efdf1ed
Test run: developer-tests-workbench2: #500

  • Adds styling to the toggle button bar.
  • It makes the toggle button bar always visible when vertical scrolling is necessary.
  • Adds the ability to set up initial panels visibility state.
  • Adds panel maximizing capabilities for those panels that take all the available space.
  • Fixes DataExplorer/DataTable components layout by moving the height calculations to the Workbench component. This way, the component can be used as one of many panels.
  • Adds a process details component used on the details panel and as a subpanel on the process view.
  • Adds the process logs component as a subpanel (WIP).
  • Adds tests.

Panels & buttons are placed in the same order. I've added the Details panel with a title as an example, so it's left to the MPVContainer component user to add it or not (take into account that titles would use precious screen real state)

The log panel is pending because I think I'll have to do some serious refactoring, and this ticket isn't about that, so I'm trying to avoid scope creeping too much. I can take it out and leave it for another ticket if you agree.

Also added a 2nd demo video of 50secs for you to have a first glance.

Actions #9

Updated by Lucas Di Pentima 9 months ago

Regarding panel sizing, I left it up to the MPVContainer user by offering the MPVPanelContent component that's just a Grid item with specific features.

For the specific case of the process panel: I think it doesn't make sense to give more space to subpanels that they need, so that's why almost all have the xs="auto" prop. For the particular case of the subprocesses panel, I think we want to take all the available space; that's why it has the xs=true prop set.

Actions #10

Updated by Lucas Di Pentima 9 months ago

Updates at arvados-workbench2|f6513f9d
Test run: developer-tests-workbench2: #501

As discussed on the eng-meeting, I've removed the Logs panel from the process view and added the Multi-View Panel to the collection view, replacing the expansion panels. Further work will be needed to add the close buttons and make the file browser layout behave correctly (right now, it has a minimum height setting that I couldn't quickly remove.

Actions #11

Updated by Lucas Di Pentima 9 months ago

Fixes test at arvados-workbench2|2b84eee4 by making the properties panel visible by default
Test run: developer-tests-workbench2: #502

Actions #12

Updated by Lucas Di Pentima 8 months ago

We've received feedback from a couple of demo sessions showing concerns about this "taskbar" being something that users would understand.
One good suggestion we've got is to make the toggle buttons mainly for navigation purposes (i.e., if scrolling is needed, clicking on it would make the corresponding panel visible) and maybe handle the hiding functionality in some other way.

Actions #13

Updated by Lucas Di Pentima 8 months ago

  • Target version changed from 2021-10-27 sprint to 2021-11-10 sprint
Actions #14

Updated by Peter Amstutz 8 months ago

  • Related to Story #16945: WB2 Workflows / containers feature parity added
Actions #15

Updated by Peter Amstutz 8 months ago

  • Release set to 46
Actions #16

Updated by Lucas Di Pentima 8 months ago

  • Target version changed from 2021-11-10 sprint to 2021-11-24 sprint
Actions #17

Updated by Lucas Di Pentima 8 months ago

I have a working PoC on the changes based on the feedback received on both demos.

Instead of making the buttons work as toggles, I've set them to allow showing hidden panels (still indicating their visibility with the eye icon). Also, signaling which panel corresponds to every button by "illuminating" each panel and conditionally scrolling to it every time the mouse pointer is over each button.

Attached is a demo video for easy viewing.

Actions #18

Updated by Lucas Di Pentima 8 months ago

Updates at arvados-workbench2|84e7cf9
Test run: developer-tests-workbench2: #515

  • Adds panel indication when mouse hovering over the panel buttons bar.
  • Instead of making the buttons toggle the visible state of the panels, they just show them on click (when hidden, or else it's a no-op)
  • If there are enough visible panel to need vertical scrolling, when mouse hovering over every button, the viewport will scroll to the selected panel.

After fiddling for some time with react & css, I couldn't make the MPVContent place the close [X] button alongside the others, so I just made this new behavior always present, and the rest of the things should be implemented by the panel developer.

The collection panel has a partial implementation of the MPVContainer: it has the task bar so that the user can navigate, but the panels don't implement the close button yet.

Actions #19

Updated by Lucas Di Pentima 7 months ago

Actions #20

Updated by Lucas Di Pentima 7 months ago

  • Target version changed from 2021-11-24 sprint to 2021-12-08 sprint
Actions #21

Updated by Peter Amstutz 7 months ago

reviewing 18128-multi-panel-view @ arvados-workbench2|e8bcd67bb51181baa6ce9b16f066e32f51574dbc

general comments:

  • Having it move to the panel on mouseover can be nice (can go between panels quickly and easily) but could also be annoying (you can end up scrolling by accident).
  • We need a follow-up ticket to add Maximize and Hide buttons to the collection panel components.
  • Would it make sense to define the Maximize and Hide buttons with a component?
  • We'll need to get feedback, the best way is to merge it and start using it.

src/components/data-explorer/data-explorer.tsx

doMaximizePanel && !!!panelMaximized &&

As mentioned on gitter I believe "!!!panelMaximized" can be written as just "!panelMaximized".

src/components/multi-panel-view/multi-panel-view.tsx

{React.cloneElement(props.children, { doHidePanel, doMaximizePanel, panelName, panelMaximized })}

What is the purpose of cloneElement() here? Is this a way of duplicating an instanced element in order to changing just some of the props?

ApiClientAuthorizationPanelRoot

Curious why this needed to change, it seems unrelated.

I've skimmed the rest of the implementation, nothing else jumped out at me. Functionally it seems to be behaving as intended, so I don't think we need to do much (if anything) to be ready to merge.

Actions #22

Updated by Lucas Di Pentima 7 months ago

Peter Amstutz wrote:

  • Having it move to the panel on mouseover can be nice (can go between panels quickly and easily) but could also be annoying (you can end up scrolling by accident).

Yes, I thought of that too. The problem is that if we don't scroll to the panel being selected by hovering over its button on the top bar, the user may not be able to see it being illuminated so the confusion of "which panel correspond to this button?" would persist. I think that if this annoys users, one solution may be to add some sort of timeout so that the auto-scrolling is done when X amount of millisecs have passed.

  • We need a follow-up ticket to add Maximize and Hide buttons to the collection panel components.

Yes, I tried to do it but found there're some difficulties so I didn't want to keep scope creeping.

  • Would it make sense to define the Maximize and Hide buttons with a component?

The subpanel layouts can be (and some currently are) very different from each other, but if we set ourselves the mission of unifying them I think it would be super convenient.

src/components/data-explorer/data-explorer.tsx

doMaximizePanel && !!!panelMaximized &&

As mentioned on gitter I believe "!!!panelMaximized" can be written as just "!panelMaximized".

Thanks, fixed.

src/components/multi-panel-view/multi-panel-view.tsx

{React.cloneElement(props.children, { doHidePanel, doMaximizePanel, panelName, panelMaximized })}

What is the purpose of cloneElement() here? Is this a way of duplicating an instanced element in order to changing just some of the props?

This is the way I found to inject optional props to a generic element without having TS complaining about it.

ApiClientAuthorizationPanelRoot

Curious why this needed to change, it seems unrelated.

There were many seemingly unrelated changes at arvados-workbench2|1ed535c - they were major layout fixing throughout the app.

I've skimmed the rest of the implementation, nothing else jumped out at me. Functionally it seems to be behaving as intended, so I don't think we need to do much (if anything) to be ready to merge.

I've fixed the triple negation and rebased at arvados-workbench2|7ef76c7
Test run: developer-tests-workbench2: #526

Actions #23

Updated by Peter Amstutz 7 months ago

Thanks. This LGTM.

Actions #24

Updated by Lucas Di Pentima 7 months ago

  • % Done changed from 0 to 100
  • Status changed from In Progress to Resolved
Actions

Also available in: Atom PDF