-
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
fix(app): Improve ODD "run again" behavior #14102
fix(app): Improve ODD "run again" behavior #14102
Conversation
Changes to TopLevelRedirect did not account for ODD "run again" routing logic. By removing the onResetSuccess callback, we avoid the superfluous redirect.
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## chore_release-7.1.0 #14102 +/- ##
=======================================================
- Coverage 70.46% 70.46% -0.01%
=======================================================
Files 2512 2512
Lines 71196 71203 +7
Branches 8958 8963 +5
=======================================================
+ Hits 50168 50172 +4
- Misses 18837 18840 +3
Partials 2191 2191
Flags with carried forward coverage won't be shown. Click here to find out more.
|
@@ -197,6 +198,19 @@ export function RunSummary(): JSX.Element { | |||
}) | |||
}, []) | |||
|
|||
const RUN_AGAIN_SPINNER_TEXT = ( |
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.
This isn't as clean as I'd like. If we end up using the spinner icon in the upper-right corner as a pattern on the ODD, we should extend the large button component to support a spinner.
The only other place we use a spinner on click on the ODD is the recently run protocols page, but we don't use a button there. I think this is another spot that we could refactor to use a new large button w/ loading spinner component.
But yeah, out of scope for this PR. I'll probably ticket this for future work.
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.
I agree we should refactor large button to support a spinner!
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.
lgtm, I think we should definitely refactor LargeButton
to support a spinner and the spinner css state. Looks like LargeButton
is only used in this component so I think it'd be a quick refactor. But feel free to make a follow up ticket for that instead to do post-launch!
const DURATION_TEXT_STYLE = css` | ||
font-size: ${TYPOGRAPHY.fontSize22}; | ||
line-height: ${TYPOGRAPHY.lineHeight28}; | ||
font-weight: ${TYPOGRAPHY.fontWeightRegular}; | ||
` | ||
|
||
const RUN_AGAIN_CLICKED_STYLE = css` |
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.
it feels a bit weird to me that LargeButton
would special case a css prop for when the spinner is showing, given that LargeButton
is meant to be a design component that encompasses all the stylings. Looks like LargeButton
is only used in this component so I think its safe to move this to be directly in LargeButton
.
@@ -197,6 +198,19 @@ export function RunSummary(): JSX.Element { | |||
}) | |||
}, []) | |||
|
|||
const RUN_AGAIN_SPINNER_TEXT = ( |
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.
I agree we should refactor large button to support a spinner!
Thanks for your feedback! I didn't notice that we are using LargeButton only here... I'm going to ticket this out separately, because the more I think through it, the more it seems like this should involve some architecting. 🎨 |
Closes RQA-1972, RQA-1973
Overview
This PR adds a loading state to the "Run Again" button on the ODD.
It also fixes a routing bug in which pressing "run again" would send the user to the protocol setup page, back to the /protocols page, then back to the protocol setup page. Changes to TopLevelRedirect did not account for ODD "run again" routing logic. By removing the onResetSuccess callback, we avoid the superfluous redirect.
Current Behavior
IMG_3829.MOV
Fixed Behavior
Screen.Recording.2023-12-06.at.9.54.40.AM.mov
Test Plan
Changelog
Risk assessment
low