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.

Sign up for GitHub

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

feat(protocol-designer): add well tooltip to liquid placement modal #2550

Merged
merged 3 commits into from
Nov 2, 2018
+48 −22
Merged

feat(protocol-designer): add well tooltip to liquid placement modal #2550

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c48332e
feat(protocol-designer): add well tooltip to liquid placement modal
b-cooper Oct 24, 2018
c057f42
protect empty well
b-cooper Oct 25, 2018
55688fc
dynamic offset for different well heights
b-cooper Oct 25, 2018
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
55 changes: 36 additions & 19 deletions protocol-designer/src/components/SelectablePlate.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,10 @@ import {
} from '@opentrons/components'

import {WELL_LABEL_OFFSET} from '../constants'
import SingleLabware from '../components/SingleLabware'
import SelectionRect from '../components/SelectionRect.js'
import SingleLabware from './SingleLabware'
import SelectionRect from './SelectionRect.js'
import WellTooltip from './labware/WellTooltip'
import type {WellIngredientNames} from '../steplist/types'
import type {ContentsByWell} from '../labware-ingred/types'
import type {RectEvent} from '../collision-types'
import styles from './SelectablePlate.css'
Expand All @@ -35,6 +37,7 @@ export type Props = {
hoverable?: boolean,
makeOnMouseOverWell?: (well: string) => (e: SyntheticMouseEvent<*>) => mixed,
onMouseExitWell?: (e: SyntheticMouseEvent<*>) => mixed,
liquidNamesById: WellIngredientNames,

onSelectionMove: RectEvent,
onSelectionDone: RectEvent,
Expand All @@ -59,6 +62,7 @@ export default function SelectablePlate (props: Props) {
hoverable = true,
makeOnMouseOverWell,
onMouseExitWell,
liquidNamesById,
} = props

// NOTE: LabwareOnDeck is not selectable or hoverable
Expand Down Expand Up @@ -99,21 +103,6 @@ export default function SelectablePlate (props: Props) {
</g>
)
} else { // NOTE: Currently only selectable and hoverable (bound to redux) in LiquidPlacementModal
const getWellProps = (wellName) => {
const well = wellContents[wellName]
return {
onMouseOver: makeOnMouseOverWell && makeOnMouseOverWell(wellName),
onMouseLeave: onMouseExitWell,
selectable,
wellName,
highlighted: well.highlighted,
selected: well.selected,
error: well.error,
maxVolume: well.maxVolume,
fillColor: ingredIdsToColor(well.groupIds),
}
}

// FIXME: SelectionRect is somehow off by one in the x axis, hence the magic number
return (
<SingleLabware showLabels>
Expand All @@ -122,8 +111,36 @@ export default function SelectablePlate (props: Props) {
originXOffset={WELL_LABEL_OFFSET - 1}
originYOffset={WELL_LABEL_OFFSET}
{...{onSelectionMove, onSelectionDone}}>
<Labware labwareType={containerType} getWellProps={getWellProps} getTipProps={getTipProps} />
<LabwareLabels labwareType={containerType} />
<WellTooltip ingredNames={liquidNamesById}>
{({makeHandleMouseOverWell, handleMouseLeaveWell}) => (
<React.Fragment>
<Labware
labwareType={containerType}
getWellProps={(wellName) => {
const well = wellContents[wellName]
return {
onMouseOver: (e: SyntheticMouseEvent<*>) => {
makeOnMouseOverWell && makeOnMouseOverWell(wellName)(e)
Copy link
Contributor

@IanLondon IanLondon Oct 25, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

NONBLOCKING: Now that this component is getting more complex with 2 sets of mouse enter/leave events, the names of the event callbacks are getting confusing

makeOnMouseOverWell comes from SelectablePlate props and means "trigger selection highlighting for the well", makeHandleMouseOverWell comes from WellTooltip and means "trigger well tooltip for to show up for the well".

Not necessarily in this PR unless you want to, but at some point soon I'd like to clean the names up. Maybe we could rename change the generic event-type named props (handle/make + OnMouseOver/OnMouseLeave/OnMouseExit etc) to "action" names. Something like (loosely): showTooltipOnMouseEvent, hideTooltip, highlightOnMouseEvent, unhighlight?

Here I postfixed the fns that actually need the SyntheticMouseEvent with ...OnMouseEvent, and left it off of the fns that don't need the event. We typed them as requiring the (e: SyntheticMouseEvent) but "hide tooltip" and "remove selection highlight" don't actually use it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed, it's becoming very unclear what the responsibilities of these functions are. I'm for holding off, as a follow up refactor will include swapping this component out for our SelectableLabware Stateful component which will allow us to hide some layers of passed through props. The fns could be renamed for clarity during that refactor.

makeHandleMouseOverWell(wellName, well.ingreds)(e)
Copy link
Contributor

@IanLondon IanLondon Oct 25, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

⚠️ This is throwing TypeErrors because when you mouse over an empty well, well.ingreds is undefined.

That's due to L51 of protocol-designer/src/top-selectors/well-contents/wellContentsAllLabware.js: ingreds: __ingredientsForContainer && __ingredientsForContainer[wellName],, should be something like ingreds: (__ingredientsForContainer && __ingredientsForContainer[wellName]) || {},

On second thought I'd prefer to keep wellContentsAllLabware doing what it does, instead of constructing 384+'s of these empty {}s in wellContentsAllLabware selector (the thing I crossed out above).

Instead, we could change the WellContents type in protocol-designer/src/labware-ingred/types.js to make ingreds value a Maybe type: ingreds: ?LocationLiquidState,.

Then the type contract for wellContentsAllLabware will be met, and we will see the Flow error on makeHandleMouseOverWell that we should see

},
onMouseLeave: (e: SyntheticMouseEvent<*>) => {
onMouseExitWell && onMouseExitWell(e)
handleMouseLeaveWell(e)
},
selectable,
wellName,
highlighted: well.highlighted,
selected: well.selected,
error: well.error,
maxVolume: well.maxVolume,
fillColor: ingredIdsToColor(well.groupIds),
}
}}
getTipProps={getTipProps} />
<LabwareLabels labwareType={containerType} />
</React.Fragment>
)}
</WellTooltip>
</SelectionRect>
</SingleLabware>
)
Expand Down
2 changes: 2 additions & 0 deletions protocol-designer/src/containers/SelectablePlate.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,7 @@ function mapStateToProps (state: BaseState, ownProps: OP): SP {
wellContents: {},
containerType: '',
selectable: selectable,
liquidNamesById: {},
}
}

Expand Down Expand Up @@ -142,6 +143,7 @@ function mapStateToProps (state: BaseState, ownProps: OP): SP {
getTipProps: getTipProps || noop,
containerType: labware ? labware.type : 'missing labware',
selectable,
liquidNamesById: selectors.getLiquidNamesById(state),
}
}

Expand Down
1 change: 1 addition & 0 deletions protocol-designer/src/top-selectors/well-contents/wellContentsAllLabware.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ const _getWellContents = (
selected: selectedWells ? wellName in selectedWells : false,
maxVolume: well['total-liquid-volume'] || Infinity,
groupIds,
ingreds: __ingredientsForContainer && __ingredientsForContainer[wellName],
},
}
}, {})
Expand Down