fix(MenuList): Deal with MenuDividers in unexpected places (#2079)

* fix(MenuList): Deal with MenuDividers in unexpected places

* Fxix color usage

* MenuDivider should properly absorb props

* Snapshots

* Fix for default-margin in Safari

Co-authored-by: Elliot Park <[email protected]>

Author: Luke Bowerman

packages/components/snapshots/MenuList/Basic-snap.png

@@ -89,6 +89,7 @@ export const ListItemWrapper = styled(ListItemWrapperInternal)`
     flex: 1;
     font-size: inherit;
     font-weight: inherit;
+    margin: 0; /* safari has default margin */
     min-width: 0;
     outline: none;
     text-align: left;

packages/components/snapshots/MenuList/Menu List Spacing-snap.png

@@ -24,17 +24,17 @@
 
  */
 
+import React, { FC } from 'react'
 import styled from 'styled-components'
 import { SpaceProps, space } from '@looker/design-tokens'
+import { Divider } from '../Divider'
 
-export const MenuDivider = styled.li<SpaceProps>`
+const MenuDividerLayout: FC<{}> = (props) => (
+  <li {...props} aria-hidden="true">
+    <Divider />
+  </li>
+)
+
+export const MenuDivider = styled(MenuDividerLayout)<SpaceProps>`
   ${space}
-  border: none;
-  border-top: 1px solid ${({ theme: { colors } }) => colors.ui2};
-
-  & + &,
-  &:first-child,
-  &:last-child {
-    display: none;
-  }
 `

packages/components/src/List/ListItemWrapper.tsx

@@ -27,7 +27,7 @@
 import React, { FC, Fragment } from 'react'
 import { Story } from '@storybook/react/types-6-0'
 import { IconNames } from '@looker/icons'
-import { Box, Grid } from '../Layout'
+import { Box, Grid, Space } from '../Layout'
 import { DensityRamp } from '../List/types'
 import { MenuHeading, MenuList, MenuItem, MenuItemProps, MenuDivider } from '.'
 
@@ -139,6 +139,46 @@ export const MenuHeadingOverride = () => (
   </MenuList>
 )
 
+/**
+ * This storyshot is specifically for checking that space above the first item
+ * and space below the last item is maintained even with extraneous dividers.
+ */
+export const MenuListSpacing = () => (
+  <Space p="medium" style={{ background: '#ff8c69' }}>
+    <Box bg="background">
+      <MenuList>
+        <MenuDivider />
+        <MenuItem selected>Top Item</MenuItem>
+        <MenuItem selected>Top Item</MenuItem>
+        <MenuDivider />
+        <MenuItem selected>Bottom Item</MenuItem>
+        <MenuItem selected>Bottom Item</MenuItem>
+        <MenuDivider />
+      </MenuList>
+    </Box>
+    <Box bg="background">
+      <MenuList>
+        <MenuItem selected>Top Item</MenuItem>
+        <MenuItem selected>Top Item</MenuItem>
+        <MenuDivider />
+        <MenuItem selected>Bottom Item</MenuItem>
+        <MenuItem selected>Bottom Item</MenuItem>
+        <MenuDivider />
+      </MenuList>
+    </Box>
+    <Box bg="background">
+      <MenuList>
+        <MenuDivider />
+        <MenuItem selected>Top Item</MenuItem>
+        <MenuItem selected>Top Item</MenuItem>
+        <MenuDivider />
+        <MenuItem selected>Bottom Item</MenuItem>
+        <MenuItem selected>Bottom Item</MenuItem>
+      </MenuList>
+    </Box>
+  </Space>
+)
+
 export default {
   component: MenuList,
   title: 'MenuList',

packages/components/src/Menu/MenuDivider.tsx

@@ -28,6 +28,7 @@ import { width, WidthProps } from '@looker/design-tokens'
 import React, { forwardRef, Ref } from 'react'
 import styled from 'styled-components'
 import { List, ListProps } from '../List'
+import { Divider } from '../Divider'
 import { MenuDivider } from './MenuDivider'
 import { MenuItem } from './MenuItem'
 import { NestedMenuProvider } from './NestedMenuProvider'
@@ -55,13 +56,24 @@ export const MenuList = styled(MenuListInternal)`
   min-width: 12rem;
   overflow: auto;
 
-  ${MenuDivider},
+  > :first-child {
+    margin-top: ${({ theme }) => theme.space.xsmall};
+
+    ${Divider} {
+      display: none;
+    }
+  }
+
   ${MenuDivider} + ${MenuItem},
-  > *:first-child {
+  ${MenuItem} + ${MenuDivider} {
     margin-top: ${({ theme }) => theme.space.xsmall};
   }
 
-  > *:last-child {
+  > :last-child {
     margin-bottom: ${({ theme }) => theme.space.xsmall};
+
+    ${Divider} {
+      display: none;
+    }
   }
 `