Skip to content
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

fix(app): Home pipettes when skipping Drop Tip wizard #15947

Merged
merged 8 commits into from
Aug 12, 2024

Conversation

mjhuff
Copy link
Contributor

@mjhuff mjhuff commented Aug 9, 2024

Overview

Drop tip wizard CTAs now home the pipette if skipping the flow and close the current run context in both the "skip" and "remove tips" cases. Copy is updated throughout all drop tip flow entry points as well.

This is the first time the app directly POSTs a one-off command via a maintenance run. We do POST one-off commands elsewhere in the app, but we do so using older, less-supported endpoints. If these one-off commands become more common, we should consider creating a dedicated API for doing so (more hooks!). To solve the immediate problem now, I refactored useDropTipMaintenanceRun.

The real complexity of this PR is managing all the interactions with closing run context and ensuring other clients not engaging with the drop tip flows see/don't see the drop tip CTAs when appropriate. It's probably easiest to follow this PR commit-by-commit.

NOTE: If a user completes a run with tips attached on both pipettes intentionally (the protocol itself has tips attached for both pipettes at the end of the run), the wizard won't display for both pipettes, only the left pipette. The run context closes as soon as the home happens, so there's no second CTA. The skip command won't home the plungers, so that's safe, but current behavior is the user will have to do drop tips via the Instruments page. I imagine this is quite the edge case, but it's worth noting. I can re-address this when I update the tip detection logic shortly.

Here's a complete breakdown of all the changes:

Drop Tip Wizard: Error Recovery, Desktop and ODD

  • DT Wiz flows remain unchanged for all ER options that are not "cancel run".
  • For "cancel run", only the copy changes for what was once "Skip" to something along the lines of "skip and home pipette." The pipette will home during cancel.

Drop Tip Wizard: Post-Run, Desktop and ODD

  • If the user has tips attached AND does not see dtwiz within error recovery when doing "cancel run", they will see an unskippable modal (no X button in the upper right) that says "skip and home pipettes" and "begin drop tip wizard".
  • Once the drop tip flows are successfully completed, the run context closes.
  • If a user clicks the upper-right corner "exit" button within, the confirmation screen now contains copy alerting the user that the pipette will home after you confirm the exit.
  • Once the user confirms they want to exit, the pipette will home, and the run context will close.

Drop Tip Wizard: Instruments Page, Desktop and ODD

  • The upper-right corner exit confirmation page copy and homing behavior is the same as the post-run behavior. We do not close any sort of run context here.

Note that the drop-tip banner will no longer be present on the desktop, post-run.

Copy Changes

Note these changes are 1:1 on the ODD when applicable, but only the desktop versions are shown.

Confirm exit before completion, non-error recovery drop tip wizard
Screenshot 2024-08-08 at 7 22 16 PM

Post run

Screenshot 2024-08-08 at 7 23 11 PM

Test Plan and Hands on Testing

  • Tested every entry point for drop tip flows and verified correct behavior, routing, and appropriate "other app" responses.

Changelog

  • Skipping drop tip wizard now homes the pipette.

Review requests

  • When we home during "skip and home", we are not homing the plungers, so it's technically not a "home" home. This is probably safer, especially since when both pipettes have tips attached, the right pipette will still have tips attached (although in practice, this is the edgest of edge cases I'd think - see overview). Are others fine with this homing behavior?

Risk assessment

medium - Closing the run context explicitly after DT is new, and doing so has the potential to lead to interesting client-side behavior.

@mjhuff mjhuff requested review from ecormany, sfoster1 and a team August 9, 2024 15:28
@mjhuff mjhuff requested a review from a team as a code owner August 9, 2024 15:28
@mjhuff mjhuff requested review from shlokamin and removed request for a team and shlokamin August 9, 2024 15:28
Copy link
Contributor

@ecormany ecormany left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was under the impression that the EXEC-631 designs (not EXEC-661) had the final copy for this. Some of the current phrasing, like "Exit before completing drop tips?", has a real crash blossom quality to it. We can really tie ourselves in linguistic knots any time we use "drop tip" as a noun — I strongly prefer "dropping a tip" or "tip drop".

@mjhuff
Copy link
Contributor Author

mjhuff commented Aug 9, 2024

I was under the impression that the EXEC-631 designs (not EXEC-661) had the final copy for this..

I believe part of the issue is that the exit confirmation screen never existed in Figma, however it has existed in code since the wizard's launch (and this was the copy). I do not have the context for why this disparity existed, but design was made aware of this screen yesterday.

Would copy along the lines of "Exit before completing tip drop?" work?

Copy link
Member

@sfoster1 sfoster1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The functionality looks good to me, defer to others on the design and phrasing.

@mjhuff
Copy link
Contributor Author

mjhuff commented Aug 9, 2024

Per convo with @ecormany and Sue, there are copy updates to the confirmation screen.

@mjhuff mjhuff requested a review from ecormany August 9, 2024 20:22
Copy link
Contributor

@ecormany ecormany left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

New copy looks good, thanks!

@mjhuff mjhuff merged commit 9335894 into chore_release-8.0.0 Aug 12, 2024
34 checks passed
@mjhuff mjhuff deleted the app_drop-tip-run-context branch August 12, 2024 15:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants