refactor(labware-creator): Break out Grid and Volume sections #7768
Conversation
Kadee80
requested review from
a team,
shlokamin and
IanLondon
and removed request for
a team
Codecov Report
@@ Coverage Diff @@
## edge #7768 +/- ##
=======================================
Coverage ? 82.75%
=======================================
Files ? 342
Lines ? 22178
Branches ? 0
=======================================
Hits ? 18353
Misses ? 3825
Partials ? 0 Continue to review full report at Codecov.
|
| 'regularColumnSpacing', | ||
| ] | ||
| const { values, errors, touched } = useFormikContext<LabwareFields>() | ||
| // @ts-expect-error `includes` doesn't want to take null/undefined |
There was a problem hiding this comment.
Could be return values.labwareType == null || ['aluminumBlock', 'tubeRack'].includes(values.labwareType) ? null : ( ... ). Otherwise it's kind of unintuitive to trace out the case for labwareType being null | undefined -- is it supposed to return null or return the div?
Also L36 needs to be export const Grid = (): JSX.Element | null => {
| } | ||
| const Content = (props: ContentProps): JSX.Element => { | ||
| const { values } = props | ||
| // @ts-expect-error `includes` doesn't want to take null/undefined |
There was a problem hiding this comment.
both of these comments can be applied to the height section as well! happy to do it lemme know
| } | ||
|
|
||
| const { container } = render(<FormAlerts {...props} />) | ||
| const warning = container.querySelector('[class="alert warning"]') |
There was a problem hiding this comment.
so testing library docs explicitly call out using class selectors as a bad thing, but i do think it's important to test that we're seeing a warning vs error here. idk if folks have a better alternative to this @Opentrons/js
| throw new Error( | ||
| `Text field should have been called with footprintXDimension or footprintYDimension, instead got ${args.name} ` | ||
| ) | ||
| return <div></div> |
There was a problem hiding this comment.
as per the testing discussion in this thread (it's long I'm sorry we should really document these opinions somewhere... perhaps in coda? @mcous), I'm removing throwing an error here because it exerts unnecessary design pressure. If someone interacts with this mock in some other way that would result in the test still passing, we should not stop it from doing so.
| if (numFieldsHidden === fieldList.length) { | ||
| // all fields are hidden, don't render this Section | ||
| return null | ||
| } | ||
| } |
There was a problem hiding this comment.
I moved this logic to the beginning of each section (where it should be). I mistakenly left it inside of FormAlerts on the first pass.
|
smoke tested. LGTM |
Overview
Addresses #7707
Changelog
Review requests
Risk assessment
Low, but we should check definition generation yet again