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

refactor(app): add thermocycler prompts to calibration flow #3912

Merged
merged 7 commits into from
Aug 26, 2019

Conversation

b-cooper
Copy link
Contributor

@b-cooper b-cooper commented Aug 20, 2019

Add helpful UI through the calibration process of a protocol that involves a thermocycler.
Specifically to ensure the latch is secure and the pcr seal is placed before the run starts.

Closes #3068, Closes #3583

Review Requests

  • Upload protocol that includes TC to a robot with a TC attached
  • After confirming module is connected and the deck items are in place, you should be prompted to latch down the TC's loaded labware with a modal
  • Clicking through confirm button on that modal should bring you to labware calibration as usual.
  • When all labware is calibrated, before proceeding to run, you should be prompted to apply the pcr seal to the TC with a modal.
  • Clicking through confirm button on that modal should allow pressing "proceed to run" button as normal.

Add helpful UI through the calibration process of a protocol that involves a thermocycler.
Specifically to ensure the latch is secure and the pcr seal is placed before the run starts.

Closes #3068, Closes #3583
@b-cooper b-cooper added feature Ticket is a feature request / PR introduces a feature app Affects the `app` project ready for review labels Aug 20, 2019
@b-cooper b-cooper requested review from mcous and Kadee80 August 20, 2019 19:25
@codecov
Copy link

codecov bot commented Aug 20, 2019

Codecov Report

Merging #3912 into edge will decrease coverage by 0.06%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##             edge    #3912      +/-   ##
==========================================
- Coverage   57.53%   57.47%   -0.07%     
==========================================
  Files         819      822       +3     
  Lines       23259    23315      +56     
==========================================
+ Hits        13382    13400      +18     
- Misses       9877     9915      +38
Impacted Files Coverage Δ
components/src/modals/AlertModal.js 100% <ø> (ø) ⬆️
...p/src/components/CalibrateLabware/InfoBoxButton.js 0% <0%> (ø)
app/src/components/CalibrateLabware/InfoBox.js 0% <0%> (ø) ⬆️
app/src/components/ReviewDeck/index.js 0% <0%> (ø) ⬆️
...pp/src/components/CalibrateLabware/ProceedToRun.js 0% <0%> (ø)
app/src/shell/remote.js 0% <0%> (ø) ⬆️
...src/components/AppSettings/AdvancedSettingsCard.js 0% <0%> (ø) ⬆️
app/src/config/index.js 25% <0%> (ø) ⬆️
app/src/config/selectors.js 66.66% <0%> (ø)
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7aaad6d...900e530. Read the comment docs.

Copy link
Contributor

@mcous mcous left a comment

Choose a reason for hiding this comment

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

Code looks good. Will approve once tested on a robot

}
}

function mergeProps(stateProps: SP, dispatchProps: DP, ownProps: OP): Props {
Copy link
Contributor

Choose a reason for hiding this comment

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

Man this diff feels good

const handleClick = () => {
// $FlowFixMe: robotActions.returnTip is not typed
returnTip()
dispatch(push(`/run`))
Copy link
Contributor

Choose a reason for hiding this comment

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

[No change necessary] In this case, I think we'd more often than not use a <Link> with an onClick and to rather than dispatching a push event via connected-react-router

I'm not super satisfied with the routing situation as it exists in the app, so if you feel that way too as you're working on it, feel free to propose any changes. I'm hoping things will get better if and when react-router ever ships their hooks API

Copy link
Contributor

@mcous mcous left a comment

Choose a reason for hiding this comment

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

⚗️

@b-cooper b-cooper changed the title feat(app): add thermocycler prompts to calibration flow refactor(app): add thermocycler prompts to calibration flow Aug 26, 2019
@b-cooper b-cooper merged commit 4b2c008 into edge Aug 26, 2019
@b-cooper b-cooper deleted the app_tc-calibration-confirmations branch August 26, 2019 22:07
@mcous mcous mentioned this pull request Sep 9, 2019
20 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
app Affects the `app` project feature Ticket is a feature request / PR introduces a feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Thermocycler: Secure Labware on Module Thermocycler: Run App: Add PCR Seal
2 participants