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

feat(app): add generic run paused splash screen #14873

Merged
merged 4 commits into from
Apr 12, 2024
Merged

Conversation

mjhuff
Copy link
Contributor

@mjhuff mjhuff commented Apr 11, 2024

Closes EXEC-387

Overview

Add a fallback run paused splash screen that will show when no specific errorType is handled explicitly. The exact logic used to display the splash message will most likely change (and we'll add specific error type subtext later).

Screenshot 2024-04-11 at 1 41 31 PM

Test Plan

  • Make sure the ODD enable run notes FF is on.
  • curl --location '<robot ip>:31950/settings' \ --header 'opentrons-version: *' \ --header 'Content-Type: application/json' \ --header 'Accept: application/json' \ --data '{ "id": "enableErrorRecoveryExperiments", "value": true }' (Please set this to false when you're done testing).
  • Run anything that will throw a pick up tip failed error (make sure edge is pushed). Verify that the splash screen appears (note that cancelling the run after this point doesn't work currently).

Changelog

  • Added "Run paused" splash screen.

Risk assessment

low

@mjhuff mjhuff requested a review from a team April 11, 2024 17:47
@mjhuff mjhuff requested a review from a team as a code owner April 11, 2024 17:47
@mjhuff mjhuff requested review from koji and removed request for a team April 11, 2024 17:47
Comment on lines +33 to +37
let subText: string | null
switch (errorType) {
default:
subText = protocolName ?? null
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

can't this be simplified to just const subText = protooclName ?? null?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The intention is we will very shortly add more errorTypes to the list.

Copy link
Contributor

Choose a reason for hiding this comment

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

question

what is the purpose of using switch here?
in the future displaying something here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, exactly. See PR description.

Copy link
Member

Choose a reason for hiding this comment

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

Reviving this - would love to see this factored into an independent, pure, exported-for-testability function e.g.

export /* for testability */ function subtext(props: RunPausedSplashProps, /* may require future extra args */): string | null {
   switch(...) return (...)
   return null
}

export function RunPausedSplash(...): ... {
    return (.. <SplashBody>{subtext(props)}</SplashBody>...)
}

This seems minor, but I know that we use the open-coded let+switch in a lot of places and then test everything with rendered-dom inspection later, and in my view it makes the component functions really big and hard to follow. I think the pattern of factoring out functions and testing them as independent units can be nicer.

@mjhuff mjhuff force-pushed the app_add-run-paused-splash branch from 4479a13 to 4c583ab Compare April 11, 2024 18:20
Copy link
Contributor

@koji koji left a comment

Choose a reason for hiding this comment

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

LGTM!

@mjhuff
Copy link
Contributor Author

mjhuff commented Apr 11, 2024

@jerader @koji Thank you both for reviewing!

@mjhuff mjhuff merged commit 80222d9 into edge Apr 12, 2024
20 checks passed
@mjhuff mjhuff deleted the app_add-run-paused-splash branch April 12, 2024 14:13
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.

Have some code-style stuff.

Comment on lines +33 to +37
let subText: string | null
switch (errorType) {
default:
subText = protocolName ?? null
}
Copy link
Member

Choose a reason for hiding this comment

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

Reviving this - would love to see this factored into an independent, pure, exported-for-testability function e.g.

export /* for testability */ function subtext(props: RunPausedSplashProps, /* may require future extra args */): string | null {
   switch(...) return (...)
   return null
}

export function RunPausedSplash(...): ... {
    return (.. <SplashBody>{subtext(props)}</SplashBody>...)
}

This seems minor, but I know that we use the open-coded let+switch in a lot of places and then test everything with rendered-dom inspection later, and in my view it makes the component functions really big and hard to follow. I think the pattern of factoring out functions and testing them as independent units can be nicer.

lastAnimatedCommand={lastAnimatedCommand.current}
updateLastAnimatedCommand={(newCommandKey: string) =>
(lastAnimatedCommand.current = newCommandKey)
{enableSplash &&
Copy link
Member

Choose a reason for hiding this comment

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

this optionality is getting kind of tough to follow. could we

  1. factor the common parts into a common function
  2. put the unique parts, I guess with uniqueness defined by the feature flag, in independent functions that don't in and of themselves check the feature flag
  3. have this component check the feature flag and delegate?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah sounds good!

@mjhuff
Copy link
Contributor Author

mjhuff commented Apr 12, 2024

#14873 (comment)

Can't respond to this in-line for some reason. I agree with you - this isn't a pattern we have used extensively, and I think that's historically because of how React used to work with the stack reconciler, back when I'm guessing we first used React. I think we can make the case pretty easily for it if we pull this out for use in the same file, and I'm fine doing this if the same-file approach works with you.

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.

4 participants