-
Notifications
You must be signed in to change notification settings - Fork 178
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): update Magnetic Module step form #16362
Conversation
protocol-designer/src/pages/Designer/ProtocolSteps/StepForm/StepTools/MagnetTools/index.tsx
Show resolved
Hide resolved
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 good to me! nice work! Feel free to merge this in after the checks are done
protocol-designer/src/pages/Designer/ProtocolSteps/StepForm/StepTools/MagnetTools/index.tsx
Outdated
Show resolved
Hide resolved
...-designer/src/pages/Designer/ProtocolSteps/StepForm/StepTools/__tests__/MagnetTools.test.tsx
Outdated
Show resolved
Hide resolved
...-designer/src/pages/Designer/ProtocolSteps/StepForm/StepTools/__tests__/MagnetTools.test.tsx
Show resolved
Hide resolved
protocol-designer/src/components/StepEditForm/forms/__tests__/MagnetForm.test.tsx
Outdated
Show resolved
Hide resolved
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.
left a few comments
lgtm
</Flex> | ||
</ListItem> | ||
</Flex> | ||
<Box borderBottom={`1px solid ${COLORS.grey30}`} /> |
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.
maybe you can use Divider
.
screen.getByText('Module') | ||
screen.getByText('mock name') | ||
screen.getByText('Magnet action') | ||
screen.getByText('Engage') |
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.
MagnetTools is https://www.figma.com/design/WbkiUyU8VhtKz0JSuIFA45/Feature%3A-Protocol-Designer-Phase-1?node-id=10435-669248&m=dev, isn't it?
If so, the copies are slightly different.
I think if you can issue a ticket that has info about what things we need to update, you can merge this PR into edge when solving CI checks.
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 you mean something like updating the step name from 'Magnet' to 'Magnetic'?
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
maybe MagneticTools
?
@ncdiehl11 any suggestion?
re AUTH-812
Overview
Updating the Magnetic module step form for single module to match the design
Test Plan and Hands on Testing
Changelog
caption
prop toToggleExpandStepFormField
for displaying the min, max, and recommended engage height text under the input fieldonToggleUpdateValue()
toToggleExpandStepFormField
to handle both boolean and string values, toggling between 'engage' and 'disengage' action typesToolbox.tsx
and other step forms to add or remove the borderBottom grey lineReview requests
Risk assessment