feat(protocol-designer): add air gap form validation #6226
Conversation
shlokamin
force-pushed
the
pd_air-gap-validation
branch
from
43a5b67 to
11f8b31
Compare
Codecov Report
|
| @@ -48,11 +48,11 @@ describe('no-op cases should pass through the patch unchanged', () => { | |||
|
|
|||
| it('empty patch', () => { | |||
| const patch = {} | |||
| expect(handleFormHelper(patch, minimalBaseForm)).toBe(patch) | |||
| expect(handleFormHelper(patch, minimalBaseForm)).toEqual(patch) | |||
There was a problem hiding this comment.
I think to toBe here was making sure that the object wasn't duplicated unnecessarily. Eg if you had a reducer that did return {...prevState}, the selectors on that reducer wouldn't be able to use shallow memoization because the state is being replaced each time. I'm not sure if there is any memoization on the form data itself directly, but if we avoid duplicating the form in cases where it doesn't change, we at least make shallow memoization possible for the form data if we need it later.
Also, the form goes thru handleFormChange on each key press when the user is typing. So performance might be significant here
There was a problem hiding this comment.
Ahh okay, thanks for pointing that out
shlokamin
force-pushed
the
pd_air-gap-validation
branch
from
b99fd71 to
391f79a
Compare
|
@Kadee80 fixed! Thanks to @IanLondon for sorting that out with me |
|
Cool, I'll merge this for now and we can make changes in a follow up PR if need be. Thanks for raising that @IanLondon! |
Overview
This PR closes #6007 by adding air gap validation to PD.
Note: the min air gap volume is "enforced" via a form level warning. The "masking" (which is done by
dependentFieldsUpdateMoveLiquid.js) only clamps the air gap volume to a value between 0 and the max air gap volume defined in the ticket.Changelog
Review requests
Code review, including tests!
pipette capacity - min pipette volume), the value should be clamped to (pipette capacity - min pipette volume)Risk assessment
Low, adding validation behind FF