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): oDD banners for pipette calibration warning #13624

Merged
merged 3 commits into from
Sep 21, 2023

Conversation

smb2268
Copy link
Contributor

@smb2268 smb2268 commented Sep 21, 2023

fix RAUT-737

Overview

Add pipette calibration warning to Instruments tab and protocol setup/instruments on ODD

Test Plan

This warning should occur on ODD protocol run setup and instruments dashboard if two pipettes are attached that have calibration values differing by more than 1.5mm
I've tested this out with mock data on my dev server, here are screenshots:
Screen Shot 2023-09-21 at 5 40 53 PM
Screen Shot 2023-09-21 at 5 40 38 PM

Changelog

  1. Create ODD version of pipette calibration warning banner and display on ODD Instruments Dashboard and Protocol Setup
  2. Create util for logic of when to show this warning so it isn't replicated across five locations
  3. Removed unused pipette calibration data and prop that I noticed hanging around in ProtocolInstrumentMountItem

Review requests

Look over the code and test on an ODD if you have time!

Risk assessment

Low

@smb2268 smb2268 requested a review from a team as a code owner September 21, 2023 22:14
@smb2268 smb2268 requested review from ncdiehl11, sfoster1, jerader, mjhuff, brenthagen and a team and removed request for a team September 21, 2023 22:14
@codecov
Copy link

codecov bot commented Sep 21, 2023

Codecov Report

Merging #13624 (70b4da4) into chore_release-7.0.0 (6f2a985) will increase coverage by 0.02%.
Report is 2 commits behind head on chore_release-7.0.0.
The diff coverage is n/a.

Additional details and impacted files

Impacted file tree graph

@@                   Coverage Diff                   @@
##           chore_release-7.0.0   #13624      +/-   ##
=======================================================
+ Coverage                71.31%   71.34%   +0.02%     
=======================================================
  Files                     2425     1588     -837     
  Lines                    68200    52793   -15407     
  Branches                  7951     3443    -4508     
=======================================================
- Hits                     48637    37664   -10973     
+ Misses                   17700    14594    -3106     
+ Partials                  1863      535    -1328     
Flag Coverage Δ
app 44.12% <ø> (-24.78%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

see 837 files with indirect coverage changes

Comment on lines +62 to +64
{getShowPipetteCalibrationWarning(instrumentsQueryData) && (
<PipetteRecalibrationWarning />
)}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
{getShowPipetteCalibrationWarning(instrumentsQueryData) && (
<PipetteRecalibrationWarning />
)}
{getShowPipetteCalibrationWarning(instrumentsQueryData) ? (
<PipetteRecalibrationWarning />
): null}

Comment on lines +48 to +49
width={SPACING.spacing32}
height={SPACING.spacing32}
Copy link
Contributor

@koji koji Sep 21, 2023

Choose a reason for hiding this comment

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

We avoid using SPACING for width, height, and size.

Suggested change
width={SPACING.spacing32}
height={SPACING.spacing32}
size="3rem"

Screenshot 2023-09-21 at 6 32 58 PM

<Icon
name="alert-circle"
color={COLORS.yellow2}
width="45px"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
width="45px"
size="1.75rem"

Screenshot 2023-09-21 at 6 31 14 PM

@@ -0,0 +1,56 @@
import * as React from 'react'
Copy link
Member

Choose a reason for hiding this comment

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

file should probably be PipetteRecalibrationODDWarning

Copy link
Contributor

@koji koji Sep 21, 2023

Choose a reason for hiding this comment

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

: []
if (failuresList.length > 0) {
return failuresList[0]?.kind === INCONSISTENT_PIPETTE_OFFSET
} else return false
Copy link
Contributor

Choose a reason for hiding this comment

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

nit

Suggested change
} else return false
} else { return false }

} from '@opentrons/components'
import { StyledText } from '../../atoms/text'

export const PipetteRecalibrationODDWarning = (): JSX.Element | null => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
export const PipetteRecalibrationODDWarning = (): JSX.Element | null => {
export function PipetteRecalibrationODDWarning(): JSX.Element | null {

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.

Tested on a machine and works!

@sfoster1 sfoster1 merged commit 92875b4 into chore_release-7.0.0 Sep 21, 2023
@sfoster1 sfoster1 deleted the app_odd-pipette-cal-warning branch September 21, 2023 23:36
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