Arvados

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.

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.

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.

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.

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

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.

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.

Updates at arvados-workbench2|e8bcd67b
Test run: developer-tests-workbench2: #517

• Fixes unit test.

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.

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

Thanks. This LGTM.