Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor(components, protocol-designer): menuList and MenuItem cleanup for helix #17257

Open
wants to merge 5 commits into
base: edge
Choose a base branch
from
Open
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 12 additions & 2 deletions components/src/atoms/MenuList/MenuItem.stories.tsx
Original file line number Diff line number Diff line change
@@ -1,12 +1,21 @@
import { STYLE_PROPS } from '../../primitives'
import { VIEWPORT } from '../../ui-style-constants'
import { MenuItem as MenuItemComponent } from './MenuItem'

import type { Meta, StoryObj } from '@storybook/react'

const meta: Meta<typeof MenuItemComponent> = {
title: 'Library/Atoms/MenuItem',
title: 'Helix/Atoms/MenuItem',
component: MenuItemComponent,
parameters: VIEWPORT.touchScreenViewport,
argTypes: {
// Disable all StyleProps
...Object.fromEntries(
[...STYLE_PROPS, 'as', 'ref', 'theme', 'forwardedAs'].map(prop => [
prop,
{ table: { disable: true } },
])
),
},
}
export default meta

@@ -16,5 +25,6 @@ export const MenuItem: Story = {
args: {
children: 'Example menu btn',
disabled: false,
isAlert: false,
},
}
7 changes: 6 additions & 1 deletion components/src/atoms/MenuList/MenuItem.tsx
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import styled from 'styled-components'
import { COLORS } from '../../helix-design-system'
import { BORDERS, COLORS } from '../../helix-design-system'
import { RESPONSIVENESS, SPACING, TYPOGRAPHY } from '../../ui-style-constants'
import { ALIGN_CENTER } from '../../styles'
import type { StyleProps } from '../../primitives'
@@ -28,6 +28,11 @@ export const MenuItem = styled.button<ButtonProps>`
color: ${COLORS.grey40};
}
&:focus-visible {
outline: 3px ${BORDERS.styleSolid} ${COLORS.blue50};
outline-offset: 2px;
}
@media ${RESPONSIVENESS.touchscreenMediaQuerySpecs} {
align-items: ${ALIGN_CENTER};
text-align: ${TYPOGRAPHY.textAlignCenter};
26 changes: 19 additions & 7 deletions components/src/atoms/MenuList/MenuList.stories.tsx
Original file line number Diff line number Diff line change
@@ -1,16 +1,27 @@
import { action } from '@storybook/addon-actions'
import { Flex, STYLE_PROPS } from '../../primitives'
import { DIRECTION_COLUMN } from '../../styles'
import { MenuList as MenuListComponent } from './index'
import { MenuItem } from './MenuItem'

import type { Meta, StoryObj } from '@storybook/react'

const menuBtn = 'example menu btn'
const menuBtn = 'Example menu btn'

const meta: Meta<typeof MenuListComponent> = {
title: 'Library/Atoms/MenuList',
title: 'Helix/Atoms/MenuList',
component: MenuListComponent,
args: {
onClick: action('clicked'),
argTypes: {
// Disable all StyleProps
...Object.fromEntries(
[
...STYLE_PROPS,
'as',
'ref',
'theme',
'forwardedAs',
'className',
].map(prop => [prop, { table: { disable: true } }])
),
},
}

@@ -21,11 +32,12 @@ type Story = StoryObj<typeof MenuListComponent>
export const MenuList: Story = {
args: {
children: (
<>
<Flex flexDirection={DIRECTION_COLUMN}>
<MenuItem>{menuBtn}</MenuItem>
<MenuItem>{menuBtn}</MenuItem>
<MenuItem>{menuBtn}</MenuItem>
</>
</Flex>
),
isOnDevice: false,
},
}
78 changes: 48 additions & 30 deletions components/src/atoms/MenuList/index.tsx
Original file line number Diff line number Diff line change
@@ -1,50 +1,68 @@
import { forwardRef } from 'react'
import { BORDERS, COLORS } from '../../helix-design-system'
import {
DIRECTION_COLUMN,
FLEX_MAX_CONTENT,
JUSTIFY_CENTER,
POSITION_ABSOLUTE,
} from '../../styles'
import { Flex } from '../../primitives'
import { SPACING } from '../../ui-style-constants'
import { ModalShell } from '../../modals'
import type { ForwardedRef, MouseEventHandler, ReactNode } from 'react'
import type { StyleProps } from '../../primitives'

import type { MouseEventHandler, ReactNode } from 'react'

interface MenuListProps {
interface MenuListProps extends StyleProps {
children: ReactNode
isOnDevice?: boolean
onClick?: MouseEventHandler
/** Optional ref - used in PD for overflowY */
ref?: ForwardedRef<HTMLInputElement>
}

export const MenuList = (props: MenuListProps): JSX.Element | null => {
const { children, isOnDevice = false, onClick = null } = props
return isOnDevice && onClick != null ? (
<ModalShell
borderRadius={BORDERS.borderRadius16}
width="max-content"
onOutsideClick={onClick}
>
export const MenuList = forwardRef<HTMLDivElement, MenuListProps>(
(props, ref): JSX.Element => {
const {
children,
isOnDevice = false,
onClick,
top = '2.6rem',
right = `calc(50% + ${SPACING.spacing4})`,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should avoid using calc for a prop.

width = FLEX_MAX_CONTENT,
zIndex = 10,
...restProps
} = props
return isOnDevice ? (
<ModalShell
borderRadius={BORDERS.borderRadius16}
width={FLEX_MAX_CONTENT}
onOutsideClick={onClick}
>
<Flex
boxShadow={BORDERS.shadowSmall}
flexDirection={DIRECTION_COLUMN}
justifyContent={JUSTIFY_CENTER}
>
{children}
</Flex>
</ModalShell>
) : (
<Flex
boxShadow={BORDERS.shadowSmall}
borderRadius={BORDERS.borderRadius8}
zIndex={zIndex}
boxShadow="0px 1px 3px rgba(0, 0, 0, 0.2)"
position={POSITION_ABSOLUTE}
backgroundColor={COLORS.white}
top={top}
right={right}
flexDirection={DIRECTION_COLUMN}
justifyContent={JUSTIFY_CENTER}
width={width}
onClick={onClick}
ref={ref}
{...restProps}
>
{children}
</Flex>
</ModalShell>
) : (
<Flex
borderRadius="4px 4px 0px 0px"
zIndex={10}
boxShadow="0px 1px 3px rgba(0, 0, 0, 0.2)"
position={POSITION_ABSOLUTE}
backgroundColor={COLORS.white}
top="2.6rem"
right={`calc(50% + ${SPACING.spacing4})`}
flexDirection={DIRECTION_COLUMN}
width="max-content"
>
{children}
</Flex>
)
}
)
}
)
2 changes: 1 addition & 1 deletion components/src/primitives/style-props.ts
Original file line number Diff line number Diff line change
@@ -106,7 +106,7 @@ const POSITION_PROPS = [

const TRANSITION_PROPS = ['transition'] as const

const STYLE_PROPS = [
export const STYLE_PROPS = [
...COLOR_PROPS,
...TYPOGRAPHY_PROPS,
...SPACING_PROPS,
12 changes: 3 additions & 9 deletions protocol-designer/src/pages/Designer/LiquidsOverflowMenu.tsx
Original file line number Diff line number Diff line change
@@ -4,7 +4,6 @@ import { useDispatch, useSelector } from 'react-redux'
import { useLocation } from 'react-router-dom'
import {
ALIGN_CENTER,
BORDERS,
COLORS,
CURSOR_POINTER,
DIRECTION_COLUMN,
@@ -13,8 +12,8 @@ import {
Icon,
LiquidIcon,
MenuItem,
MenuList,
OVERFLOW_AUTO,
POSITION_ABSOLUTE,
SPACING,
StyledText,
TYPOGRAPHY,
@@ -42,17 +41,12 @@ export function LiquidsOverflowMenu(
const { t } = useTranslation(['starting_deck_state'])
const liquids = useSelector(labwareIngredSelectors.allIngredientNamesIds)
const dispatch: ThunkDispatch<any> = useDispatch()

return (
<Flex
position={POSITION_ABSOLUTE}
<MenuList
zIndex={12}
right={location.pathname === '/liquids' ? SPACING.spacing12 : '3.125rem'}
top={`calc(${NAV_HEIGHT} - 6px)`}
ref={overflowWrapperRef}
borderRadius={BORDERS.borderRadius8}
boxShadow="0px 1px 3px rgba(0, 0, 0, 0.2)"
backgroundColor={COLORS.white}
flexDirection={DIRECTION_COLUMN}
onClick={(e: MouseEvent) => {
e.preventDefault()
@@ -113,6 +107,6 @@ export function LiquidsOverflowMenu(
</StyledText>
</Flex>
</MenuItem>
</Flex>
</MenuList>
)
}
Original file line number Diff line number Diff line change
@@ -1,16 +1,7 @@
import { useTranslation } from 'react-i18next'
import { useDispatch, useSelector } from 'react-redux'

import {
BORDERS,
COLORS,
DIRECTION_COLUMN,
Divider,
Flex,
MenuItem,
NO_WRAP,
POSITION_ABSOLUTE,
} from '@opentrons/components'
import { Divider, MenuItem, MenuList, NO_WRAP } from '@opentrons/components'
import { analyticsEvent } from '../../../../analytics/actions'
import { actions as stepsActions } from '../../../../ui/steps'
import {
@@ -26,7 +17,12 @@ import {
getUnsavedForm,
} from '../../../../step-forms/selectors'

import type { Dispatch, MutableRefObject, SetStateAction } from 'react'
import type {
Dispatch,
MouseEvent,
MutableRefObject,
SetStateAction,
} from 'react'
import type { ThunkDispatch } from 'redux-thunk'
import type { BaseState } from '../../../../types'
import type { StepIdType } from '../../../../form-types'
@@ -97,17 +93,13 @@ export function StepOverflowMenu(props: StepOverflowMenuProps): JSX.Element {

return (
<>
<Flex
ref={menuRootRef}
<MenuList
zIndex={12}
ref={menuRootRef}
top={top}
left="18.75rem"
position={POSITION_ABSOLUTE}
right={undefined}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

need this?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

probably we should avoid passing undefined by set the default value in MenuList.

whiteSpace={NO_WRAP}
borderRadius={BORDERS.borderRadius8}
boxShadow="0px 1px 3px rgba(0, 0, 0, 0.2)"
backgroundColor={COLORS.white}
flexDirection={DIRECTION_COLUMN}
onClick={(e: MouseEvent) => {
e.preventDefault()
e.stopPropagation()
@@ -173,7 +165,7 @@ export function StepOverflowMenu(props: StepOverflowMenuProps): JSX.Element {
</MenuItem>
</>
)}
</Flex>
</MenuList>
</>
)
}

Unchanged files with check annotations Beta

units?: string
type?: string
}
interface PipetteQuirksField {

Check warning on line 62 in api-client/src/pipettes/types.ts

GitHub Actions / js checks

A record is preferred over an index signature

Check warning on line 62 in api-client/src/pipettes/types.ts

GitHub Actions / js checks

A record is preferred over an index signature
[quirkId: string]: boolean
}
interface QuirksField {
quirks?: PipetteQuirksField
}
export type PipetteSettingsFieldsMap = QuirksField & {

Check warning on line 69 in api-client/src/pipettes/types.ts

GitHub Actions / js checks

A record is preferred over an index signature

Check warning on line 69 in api-client/src/pipettes/types.ts

GitHub Actions / js checks

A record is preferred over an index signature
[fieldId: string]: PipetteSettingsField
}
export interface IndividualPipetteSettings {
fields: PipetteSettingsFieldsMap
}
type PipetteSettingsById = Partial<{ [id: string]: IndividualPipetteSettings }>

Check warning on line 77 in api-client/src/pipettes/types.ts

GitHub Actions / js checks

A record is preferred over an index signature

Check warning on line 77 in api-client/src/pipettes/types.ts

GitHub Actions / js checks

A record is preferred over an index signature
export type PipetteSettings = PipetteSettingsById
export interface PipetteSettingsUpdateFieldsMap {

Check warning on line 81 in api-client/src/pipettes/types.ts

GitHub Actions / js checks

A record is preferred over an index signature

Check warning on line 81 in api-client/src/pipettes/types.ts

GitHub Actions / js checks

A record is preferred over an index signature
[fieldId: string]: PipetteSettingsUpdateField
}
} | null
export interface UpdatePipetteSettingsData {
fields: { [fieldId: string]: PipetteSettingsUpdateField }

Check warning on line 90 in api-client/src/pipettes/types.ts

GitHub Actions / js checks

A record is preferred over an index signature

Check warning on line 90 in api-client/src/pipettes/types.ts

GitHub Actions / js checks

A record is preferred over an index signature
}
export interface ResourceLink {
href: string
meta?: Partial<{ [key: string]: string | null | undefined }>

Check warning on line 14 in api-client/src/types.ts

GitHub Actions / js checks

A record is preferred over an index signature

Check warning on line 14 in api-client/src/types.ts

GitHub Actions / js checks

A record is preferred over an index signature
}
export type ResourceLinks = Record<
export const appRestart = (message: string): AppRestartAction => ({
type: APP_RESTART,
payload: {
message: message,

Check warning on line 360 in app-shell-odd/src/actions.ts

GitHub Actions / js checks

Expected property shorthand

Check warning on line 360 in app-shell-odd/src/actions.ts

GitHub Actions / js checks

Expected property shorthand
},
meta: { shell: true },
})
export const reloadUi = (message: string): ReloadUiAction => ({
type: RELOAD_UI,
payload: {
message: message,

Check warning on line 368 in app-shell-odd/src/actions.ts

GitHub Actions / js checks

Expected property shorthand

Check warning on line 368 in app-shell-odd/src/actions.ts

GitHub Actions / js checks

Expected property shorthand
},
meta: { shell: true },
})
export const sendLog = (message: string): SendLogAction => ({
type: SEND_LOG,
payload: {
message: message,

Check warning on line 376 in app-shell-odd/src/actions.ts

GitHub Actions / js checks

Expected property shorthand

Check warning on line 376 in app-shell-odd/src/actions.ts

GitHub Actions / js checks

Expected property shorthand
},
meta: { shell: true },
})
export const updateBrightness = (message: string): UpdateBrightnessAction => ({
type: UPDATE_BRIGHTNESS,
payload: {
message: message,

Check warning on line 384 in app-shell-odd/src/actions.ts

GitHub Actions / js checks

Expected property shorthand

Check warning on line 384 in app-shell-odd/src/actions.ts

GitHub Actions / js checks

Expected property shorthand
},
meta: { shell: true },
})