Arvados

developer-run-tests-services-workbench2: #1009

21720-material-ui-upgrade @ abe78f4411b8145fc7a501cee58c2de1f9b63042

• ✅ All agreed upon points are implemented / addressed.
• ✅ Anything not implemented (discovered or discussed during work) has a follow-up story.
• ✅ Code is tested and passing, both automated and manual, what manual testing was done is described
• ✅ Documentation has been updated.
  - https://dev.arvados.org/projects/arvados/wiki/Running_tests could be updated to reflect the new 'yarn test-path` script, but that's not strictly necessary. The existing `yarn test` script will work just fine.
• ✅ Behaves appropriately at the intended scale (describe intended scale).
• ✅ Considered backwards and forwards compatibility issues between client and server.
  - no changes
• ✅ Follows our coding standards and GUI style guidelines.

• new `yarn test-path "src/path/to/test"` script
• search-bar-view.test.tsx has basically no tests
• auth-action test is incomplete
• there may be minor css flaws here and there, as theme.spacing had to be recalculated. I tried to catch them all, but it's likelly a couple slipped through.
• several browser warnings (mostly pre-existing) still need to be cleaned up, possibly on a future ticket

• I noticed that Peter recently added some styles to run-process-basic-form that needs migration - withStyles and StyleRulesCallback
• I also happened to create a new unit test a while back in #21764 (tree-picker.test.tsx) that looks like isn't in your branch - if it's easy to merge in main and convert that test too that would be great, otherwise that can be handled in a separate ticket.

• The top right menu popovers have a gap between the icon and the popover - it seems changing the transformOrigin vertical value to 0 in dropdown-menu.tsx fixes this. Maybe where transformOrign is relative to changed with the upgrade
• 2 of the context menu actions, file-viewer-action.tsx and download-action.tsx, have blue text styling - maybe the ListItem or another element used to override the link text color but changed. Simply adding color: 'inherit' to the anchor of each one is a quick fix
• The context menu has some extra spacing due to a min-width: 56px, removing that from the icon root and adding back margin-right: 16px; seems to resolve that - I'm not sure where these styles reside.
• There are some other subtle styling differences in the context menu, like font-size 13px/0.8125rem (I'm guessing that has to do with the global 0.8125rem that started to apply to more things and needed to be changed to 0.875rem), and line-height: 1.5, and the cy=context-menu wrapper used to have 4px top/bottom padding but is now 8px, and the spacing of the items can be better matched to what it was before by changing the padding-top/bottom of each item wrapper to 6px (it's 4 now, and originally 8, but idk why there's too much space if it's reverted to 8 but 6px top/bottom gets close to the original spacing)
• The main background seems to have lost its gray color - it's set in workbench.tsx to theme.palette.background.default which appears to be #fafafa, not sure how that got lost but adding background: {default: '#fafafa'} to the palette section in custom-theme.ts seems to fix it.
• The filter icon on data explorers is a little out of vertical alignment. Adding line-height 100% to the i element of the icon is a possible fix.
• The collapse left side panel button gets cut off if there are items with long names - the previous behavior is to keep the collapse icon button pinned to the right side of the side panel regardless of overflow scroll.

• in 2e7aef5d90aa9f21c928ee0ba86e71f6099b1c2a I'm pretty sure those are casts not assertions, but it seems like most aren't needed anyway as the types appear to already line up. Maybe it was erroring before the upgrades were complete. The only exception seems to be the last one in process-log-form.tsx where the as string looks like it can be removed but the change to ev: any seems to fix that one. So I would say revert all of that commit except in process-log-form.tsx, use:
  

  onChange={(ev: any) => onChange({ label: ev.target.innerText, value: ev.target.value })}
  

• Some of the MuiAccordion styles don't seem to be applying in custom-theme.ts, specifically the &$expanded rules on line 133 and 146 - not sure if this has anything to do with the upgrade and I'm not sure what should be done about it since it might have some effect that I can't find. If we can verify it's not used it would be good to remove it.
• in d2ce1f4550d7e4e2605a9c0c50aca0bd549fee90 the buttonLabel class got removed from breadcrumbs.tsx which is needed to allow the breadcrumbs to collapse to just a row of icons on small widths - it seems MUI got rid of the label wrapper so we can add our own. We also need to add a couple additional styles that were previously present on the wrapper so this is my proposed solution:

diff --git a/services/workbench2/src/components/breadcrumbs/breadcrumbs.tsx b/services/workbench2/src/components/breadcrumbs/breadcrumbs.tsx
index d536ec0973..3d9defc1b0 100644
--- a/services/workbench2/src/components/breadcrumbs/breadcrumbs.tsx
+++ b/services/workbench2/src/components/breadcrumbs/breadcrumbs.tsx
@@ -46,6 +46,8 @@ const styles: CustomStyleRulesCallback<CssRules> = (theme: ArvadosTheme) => ({
     buttonLabel: {
         overflow: 'hidden',
         justifyContent: 'flex-start',
+        display: 'inherit',
+        alignItems: 'inherit',
     },
     icon: {
         fontSize: 20,
@@ -95,6 +97,7 @@ export const Breadcrumbs = withStyles(styles)(
                             color="inherit" 
                             onClick={() => onClick(navFunc, item)}
                             onContextMenu={event => onContextMenu(event, item)}>
+                            <span className={classes.buttonLabel}>
                             <Icon className={classes.icon} />
                             <Typography
                                 noWrap
@@ -105,6 +108,7 @@ export const Breadcrumbs = withStyles(styles)(
                             {
                                 (resources[item.uuid] as any)?.frozenByUuid ? <FreezeIcon className={classes.frozenIcon} /> : null
                             }
+                            </span>
                         </Button>
                     </Tooltip>
                     {!isLastItem && <ChevronRightIcon color="inherit" className={classNames('parentItem', classes.chevron)} />}

• It seems the codemod replaced lots of "- spacing(x) / 2" with -calc(${spacing(x)} / 2), which doesn't work because apparently -calc doesn't work. It's much simpler to just do spacing(-0.5) in chips.tsx - or even margin: spacing(0, -0.5), to condense it further. The same applies to select-item.tsx, virtual-machine-user-panel, and virtual-machine-admin-panel. You can easily find these with grep -ri "\-calc(" --exclude-dir=node_modules
• Although it's not super important because it works already, but there are also 4 places with a non-negative calc(${theme.spacing(1)} / 2) that can be simplified to ${theme.spacing(0.5)}. A neat feature of spacing is that it can generate multiple values, so in search-bar-view line 45, calc(${theme.spacing(1)} / 2) calc(${theme.spacing(1)} / 2) 0 0 can be condensed to borderRadius: theme.spacing(0.5, 0.5, 0, 0),, and the same borderRadius in search-bar-basic-view, search-bar-autocomplete-view, and search-bar-advanced-view can be changed to borderRadius: theme.spacing(0, 0, 0.5, 0.5),. A quick way to find the remaining usage of calc with theme.spacing is with grep -ri 'calc(\${theme' --exclude-dir=node_modules.
• As part of the migration for Grid to using justifyContent instead of justify-content, I noticed that MPVContainer accepts GridProps including justify-content and just passes those props to Grid, so the 3 usages of MPVContainer should probably have justify-content changed to justifyContent
• In form-dialog.tsx, the removal of disableBackdropClick now allows clicking out of the sharing dialog with unsaved changes, so blocking that should be ported to the new MUI somehow.
  • This also affects the not found dialog but I don't think that one needs to be fixed since as I understand it, that dialog can always be freely closed anyway
• In text-field.tsx, I noticed the removal of PropTypes leaves the margin prop as any, but I think we can still type that field because it's passed to FormControl which has a type for margin that can be accessed using FormControlOwnProps["margin"] as the type and adding FormControlOwnProps to the '@mui/material' import

• In the workflow panel actions test - I'm not sure if this is intentional but the previous test checked for name: testing on line 76 but it looks like it's checking for inputparam twice now

developer-run-tests-services-workbench2: #1103

21720-material-ui-upgrade @ 96f979714bd57ef049974365fda0d3c9148fe6fe

So since this branch has been in flight for a while, it would be good to merge...
I noticed that Peter recently added some
I also happened to create a new unit test...

I am currently working through the merge with main, and will handle those tests in turn.

From visual inspection:

• The top right menu popovers have a gap between...

Fixed, and also fixed a few other areas where the same bug appeared, like in the autocompletes

• 2 of the context menu actions, file-viewer-action.tsx and download-action.tsx, have blue text...
• The context menu has some extra spacing...
• There are some other subtle styling differences in the context menu...
• The main background seems to have lost its gray color... (I can't believe I didn't notice this!)

Fixed by setting the csx in global customTheme.ts

• The filter icon on data explorers is a little out of vertical alignment. Adding line-height 100% to the i element of the icon is a possible fix.

The fontSize of the table head labels was off, so I fixed that and added 2px of padding to straighten it out,

• The collapse left side panel button gets cut off...

This was due to some changes in the Grid API, fixed

Code suggestions:

• in 2e7aef5d90aa9f21c928ee0ba86e71f6099b1c2a...

You are correct, they were set that way in an intermediate state, and I didn't go back and change them when I was doing my normal cleanup

• Some of the MuiAccordion styles don't seem to be applying...

removed

• in d2ce1f4550d7e4e2605a9c0c50aca0bd549fee90 the buttonLabel class...

applied, just as you said

• It seems the codemod replaced lots of "- spacing(x) / 2"...
• Although it's not super important because it works already...

This was really strange, as calc() works in other places, but I replaced the broken ones with spacing(0.5)

• As part of the migration for Grid to using justifyContent...

done

• In form-dialog.tsx, the removal of disableBackdropClick...
  • This also affects the not found dialog but I don't think...

handled with an intermediate func that tests if the reason for onClose() is 'backDropClick'

• In text-field.tsx, I noticed the removal of PropTypes leaves the margin prop as any...

fixed, and that's a really good catch

Test suggestions:

• In the workflow panel actions test - I'm not sure if this is intentional...

It was not intentional, and it's fixed now.

I also made other touch-ups to css wherever I found them, and I had to come up with a mildly hacky solution to make the datagrid row hover highlight the checkboxCell as well, since the newer Grid doesn't apply it automatically anymore.

• Is there a reason to keep the min-width: 56px and use negative margin-right instead of reverting back to no min-width and margin-right: 16px for the context menu List Item Icon? The old way seems a bit more elegant
• It seems there is an info icon button now in the top right of every page that triggers an error - maybe related to the merge?
• On the process page / multi panel view, the scrollbar for the whole page overlaps with the scrollbar for single panels (for example the logs panel). The right side of the drop shadow on the panels is also cut off
  • It doesn't seem too complicated to restore the previous layout and fix these:

--- a/services/workbench2/src/components/multi-panel-view/multi-panel-view.tsx
+++ b/services/workbench2/src/components/multi-panel-view/multi-panel-view.tsx
@@ -24,6 +24,8 @@ type CssRules =
     | 'buttonIcon'
     | 'content'
     | 'exclusiveContentPaper'
+    | 'exclusiveContent'
+    | 'buttonBarGridContainer'
     | 'tabs';

 const styles: CustomStyleRulesCallback<CssRules> = theme => ({
@@ -34,7 +36,8 @@ const styles: CustomStyleRulesCallback<CssRules> = theme => ({
         marginBottom: '15px',
     },
     gridContainerRoot: {
-        marginTop: '10px',
+        margin: '10px -4px -4px',
+        width: 'calc(100% + 8px) !important',
     },
     exclusiveGridContainerRoot: {
         marginTop: 0,
@@ -58,11 +61,19 @@ const styles: CustomStyleRulesCallback<CssRules> = theme => ({
     },
     content: {
         overflow: 'auto',
-        maxWidth: 'initial', // removing this fixes overflow on the project page which uses mutuallyExclusive mode
+        margin: '-4px',
+        padding: '4px !important',
+    },
+    exclusiveContent: {
+        overflow: 'auto',
+        margin: 0,
     },
     exclusiveContentPaper: {
         boxShadow: 'none',
     },
+    buttonBarGridContainer: {
+        padding: '4px !important',
+    },
     tabs: {
         flexGrow: 1,
         flexShrink: 1,
@@ -141,7 +152,7 @@ export const MPVPanelContent = ({ doHidePanel, doMaximizePanel, doUnMaximizePane
         ? '100%'
         : maxHeight;

-    return <Grid item style={{ maxHeight: maxH, minHeight }} {...props}>
+    return <Grid item style={{ maxHeight: maxH, minHeight, padding: '4px' }} {...props}>
         <span ref={panelRef} /> {/* Element to scroll to when the panel is selected */}
         <Paper style={{ height: '100%' }} elevation={panelIlluminated ? 8 : 0}>
             {forwardProps
@@ -277,12 +288,12 @@ const MPVContainerComponent = ({ children, panelStates, classes, ...props }: MPV
             <Tabs value={currentSelectedPanel} onChange={(e, val) => showFn(val)()} data-cy={"mpv-tabs"}>
                 {tabs.map((tgl, idx) => <Tab className={classes.tabs} key={idx} label={tgl} />)}
             </Tabs> :
-            <Grid container item direction="row">
+            <Grid container item direction="row" className={classes.buttonBarGridContainer}>
                 {buttons.map((tgl, idx) => <Grid item key={idx}>{tgl}</Grid>)}
             </Grid>;
     };

-    const content = <Grid container item {...props} xs className={classes.content}
+    const content = <Grid container item {...props} xs className={props.mutuallyExclusive ? classes.exclusiveContent : classes.content}
         onScroll={() => setSelectedPanel(-1)}>
         {panelVisibility.includes(true)
             ? panels

Since MUI uses css rules of specificity 2 on the grid items I think the uses of !important are the easiest way to override the padding. I looked at using & > .Muiwhatever but the MUI documentation suggests that those class names aren't stable so I assume that solution would break when WB gets built and all classnames turn into jssXXX, I'm not sure if that also applies to using the same classes in sx.

• I noticed that on data explorers, non-link text is now gray instead of the nearly-black color
• In the top-right account menu, the separator above logout seems to have some extra padding - I think that should probably be fixed globally somehow so any use of the separator matches the old layout.
• I also noticed some missing padding on the process panel log card lines, that's a simple fix it looks like paddingY is accepted as a key but doesn't actually render padding in FF since padding-y doesn't seem to be valid. The lines also need a display: block:
  

  --- a/services/workbench2/src/views/process-panel/process-log-code-snippet.tsx
  +++ b/services/workbench2/src/views/process-panel/process-log-code-snippet.tsx
  @@ -38,7 +38,8 @@ const styles: CustomStyleRulesCallback<CssRules> = (theme: ArvadosTheme) => ({
       },
       logText: {
           color: '#fff',
  -        paddingY: theme.spacing(0.5),
  +        padding: theme.spacing(0, 0.5),
  +        display: 'block',
       },
       wordWrapOn: {
           overflowWrap: 'anywhere',
  
  

I just merged a couple small changes to the now-deleted unit tests so you will need to make a couple tweaks to the converted component tests to pass: arvados|04999b7eecdab0fac215084dee56a3e4458c7eda - mostly just changing the number of expected calls, adding an extra data explorer reducer call, and accounting for the extra count:exact request.

21720-material-ui-upgrade @ 44aa666e4918564a80fb2f14c130d3e7090adab8

developer-run-tests-services-workbench2: #1145

• Is there a reason to keep the min-width: 56px and use negative margin-right instead of reverting back to no min-width and margin-right: 16px for the context menu List Item Icon? The old way seems a bit more elegant

That was some MUI styling that I was trying to override, and you're right, the `minWidth: 0` etc is more elegant

• It seems there is an info icon button now in the top right of every page that triggers an error - maybe related to the merge?

Yeah, it slipped through the merge and I missed it on visual inspection

• On the process page / multi panel view, the scrollbar for the whole page overlaps with the scrollbar for single panels (for example the logs panel). The right side of the drop shadow on the panels is also cut off

Good catch, I implemented your changes, and thank you for the helpful suggestions

• I noticed that on data explorers, non-link text is now gray instead of the nearly-black color

This was due to an attempt to fix icon colors, which no longer inherit styles from their parent it seems? Either way, it's fixed now.

• In the top-right account menu, the separator above logout seems to have some extra padding - I think that should probably be fixed globally somehow so any use of the separator matches the old layout.

Other dividers didn't have the same issue, and I don't see a point in the barely-visible divider that was there, so I just removed it.

• I also noticed some missing padding on the process panel log card lines, that's a simple fix it looks like paddingY is accepted as a key but doesn't actually render padding in FF since padding-y doesn't seem to be valid. The lines also need a display: block:

Again, good catch. Fixed

I also merged main with this branch again to catch the recent changes and had a handful of fixes to make it all work.

Just one final tweak - in process-log-code-snippet.tsx:L41 that should be padding instead of paddingY so that the log lines have side padding.

Once that's done everything LGTM! :shipit: