-
Notifications
You must be signed in to change notification settings - Fork 179
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
feat(protocol-designer,-shared-data): add liquid class scaffolding to PD #17126
base: edge
Are you sure you want to change the base?
Conversation
This PR introduces initial work for liquid class support in protocol designer. I create a full liquid class interface based on the API's Pydantic model. I also add utilities for getting all liquid class definitions and a feature flag to hide liquid classes from the UI. Lastly, I add some of the basic functionality for assigning a liquid class to a defined liquid, and displaying that liquid class in LiquidCard. A liquid class assigned to a defined liquid will be accessible through extended existing selectors (allIngredientNamesIds) for step forms.
// ==== INGREDIENTS ==== | ||
export type OrderedLiquids = Array<{ | ||
ingredientId: string | ||
name: string | null | undefined | ||
displayColor: string | null | undefined | ||
liquidClass: string | null | undefined |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit, the other fields can be refactored as well.
liquidClass: string | null | undefined | |
liquidClass?: string | null |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this undefined here to avoid needing to add this to a migration?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think these are unnecessarily allowed to be undefined or null in some cases. The modal disallows saving without a liquid name, and displayColor is always a valid hex string. I think description can be set to null for easier use downstream as well. Thoughts on this interface for the selector allIngredientGroupFields
:
export interface LiquidGroup {
name: string
description: string | null
displayColor: string
liquidClass: string | null
serialize: boolean
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and since we're dealing with nullish initial values in DefineLiquidsModal
, we can update that interface to
interface LiquidEditFormValues {
name: string
displayColor: string
description: string
liquidClass: string
serialize: boolean
[key: string]: unknown
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i see, that makes sense to me.
i think we would need liquidClass
to be undefined in OrderedLiquids
at least because its included in the JSON protocols and if it is always null or a string, we'd need to make a migration for it. i'm all for adding this to the migration but not right now since there's no way to put it behind a FF. Do you mind adding a TODO or something above the type to remind us to migrate it later?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For sure! I will keep it as an optional property for now and we can remove the optionality and add a migration later
| 'well-top' | ||
| 'well-center' | ||
| 'liquid-meniscus' | ||
type BlowoutLocation = 'source' | 'destination' | 'trash' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we not have this type already?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see it anywhere in ts types
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks great! left some comments. curious if adding liquidClass
to the ingredients would be nice to have in the migration -- like populating it to null. though it'd be tricky to make a migration right now since we may need one in 8.3.0 or for plate reader, so i'd recommend adding a TODO for that for now if we decide to go down that route.
IngredInputs, | ||
} from '../../labware-ingred/types' | ||
|
||
const getLiquidDescription = ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for consistency
const getLiquidDescription = ( | |
function getLiquidDescription( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought we usually defined utility functions with const notation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wait i thought so too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry the comment isn't clear enough.
This returns JSX.Elment so the following method would be consistent.
function LiquidDescription()
touchTip: TouchTipProperties | ||
delay: DelayProperties | ||
} | ||
type RetractAspirate = BaseRetract |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
need this?
extends BaseLiquidHandlingProperties<BaseRetract>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think so, since the interface is exactly the same. I wanted to add a Base interface so that Aspirate and Dispense could share it
probably add ff mock code to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
awesome! 🎉
Overview
This PR introduces initial work for liquid class support in protocol designer. I create a full liquid class interface based on the API's Pydantic model. I also add utilities for getting all liquid class definitions and a feature flag to hide liquid classes from the UI. Lastly, I add some of the basic functionality for assigning a liquid class to a defined liquid, and displaying that liquid class in LiquidCard. A liquid class assigned to a defined liquid will be accessible through extended existing selectors (allIngredientNamesIds) for step forms.
closes AUTH-965
closes AUTH-968
Test Plan and Hands on Testing
Screen.Recording.2024-12-17.at.2.44.38.PM.mov
Changelog
Review requests
Risk assessment
medium