refactor(app): remove start run disablement and replace with toast #20182
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## edge #20182 +/- ##
===========================================
+ Coverage 25.89% 56.91% +31.02%
===========================================
Files 3566 3566
Lines 297498 297609 +111
Branches 42041 42278 +237
===========================================
+ Hits 77023 169378 +92355
+ Misses 220456 127957 -92499
- Partials 19 274 +255
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
mjhuff
left a comment
mjhuff
left a comment
There was a problem hiding this comment.
Nice work! Structure is good overall, just left a few functional fixes and cleanup comments.
| interface ActionButtonProps extends BaseActionButtonProps { | ||
| isResetRunLoadingRef: MutableRefObject<boolean> | ||
| isClosingCurrentRun: boolean | ||
| protocolRunHeaderRef: RefObject<HTMLDivElement> | null |
There was a problem hiding this comment.
| protocolRunHeaderRef: RefObject<HTMLDivElement> | null |
I don't think we need this?
|
|
||
| const { isDisabled, disabledReason } = useActionBtnDisabledUtils({ | ||
| isCurrentRun, | ||
| useActionBtnDisabledUtils({ |
There was a problem hiding this comment.
| useActionBtnDisabledUtils({ |
Hm, I don't think we need this here now that this functionality is encapsulated by useActionButtonProperties?
| !isValidRunAgain | ||
| ) { | ||
| return t('setup_incomplete') | ||
| if (isCalibrationComplete) { |
There was a problem hiding this comment.
| if (isCalibrationComplete) { | |
| if (!isCalibrationComplete) { |
| return t('instrument_calibration_incomplete') | ||
| } else if (isMissingModules) { | ||
| return t('modules_missing') | ||
| } else if (isModuleCalibrationComplete) { |
There was a problem hiding this comment.
| } else if (isModuleCalibrationComplete) { | |
| } else if (!isModuleCalibrationComplete) { |
| return t('module_calibration_incomplete') | ||
| } else if (isFixtureMismatch) { | ||
| return t('fixture_mismatch') | ||
| } else if (isValidRunAgain) { |
There was a problem hiding this comment.
| } else if (isValidRunAgain) { | |
| } else if (!isValidRunAgain) { |
| return t('close_door') | ||
| } else if (isClosingCurrentRun) { | ||
| return t('shared:robot_is_busy') | ||
| } else if (!isCameraReadyToRun) { |
There was a problem hiding this comment.
Nit, I think it's probably slightly better user flow to group the "click buttons on your desktop app" disabled reasons together, and then group together the "be at your robot to do stuff" reasons together. In theory, this would help reduce the amount of back and forth between robot and desktop app someone may have to do.
Since a user is going to be clicking the start run button on their desktop, can we move the "software update is available" and "enable camera", "is fixture mismatch", "is valid run again", and "robot is busy" reasons to the top of this list, and then have all the "be at your robot reasons" beneath it?
Overview
Refactor start run disablement button to display toast description.
Test Plan and Hands on Testing
Changelog
Review requests
Risk assessment
Closes EXEC-1635