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): Re-enable change pipette and pipette settings #3475

Merged
merged 3 commits into from
May 21, 2019

Conversation

Kadee80
Copy link
Contributor

@Kadee80 Kadee80 commented May 21, 2019

overview

This PR serves as a bug report and fix. A recent API update returns both a name and model for attached pipettes. The AttatchedInstrumentsCard and InstrumentInfo were using name for robot.name which caused a conflict resulting in broken routes for change pipette and pipette settings.

This PR changes name (robot) prop to robotName which fixes the pipette modal routes.

This PR also found another bug with confirming detached pipettes. That should be fixed now as well.

changelog

  • fix(app): Re-enable change pipette and pipette settings (robotName prop)
  • fix(app): Swap out pipette name in change pipette routes with React Hook.

review requests

Please test on a robot with edge installed and one with an older version of the API!

  • attached pipettes card renders as expected
  • clicking [change/attach] opens change pipette modal (please confirm change pipette walkthrough still works as expected)
  • change pipette flow is no longer broken
  • clicking [settings] for an attached pipettes pops up pipette config modal. (please confirm settings are still updatable.

Files that are not affected by this PR but should be double checked if possible:

  • file info page pipettes section is unaffected
  • tip probe pages are unaffected

@Kadee80 Kadee80 added bug app Affects the `app` project fix PR fixes a bug labels May 21, 2019
@Kadee80 Kadee80 self-assigned this May 21, 2019
@Kadee80 Kadee80 added the WIP label May 21, 2019
@codecov
Copy link

codecov bot commented May 21, 2019

Codecov Report

Merging #3475 into edge will increase coverage by 0.09%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff            @@
##            edge    #3475      +/-   ##
=========================================
+ Coverage   52.4%   52.49%   +0.09%     
=========================================
  Files        796      796              
  Lines      23194    23246      +52     
=========================================
+ Hits       12155    12204      +49     
- Misses     11039    11042       +3
Impacted Files Coverage Δ
...ponents/InstrumentSettings/AttachedPipettesCard.js 0% <ø> (ø) ⬆️
...rc/components/InstrumentSettings/InstrumentInfo.js 0% <0%> (ø) ⬆️
app/src/components/ChangePipette/index.js 0% <0%> (ø) ⬆️
shared-data/js/labwareTools/index.js 96.11% <0%> (+1.77%) ⬆️

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 3513794...c7a0615. Read the comment docs.

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.

Please test on a robot with edge installed and one with an older version of the API!

  • attached pipettes card renders as expected

  • clicking [change/attach] opens change pipette modal (please confirm change pipette walkthrough still works as expected)

  • unrelated issue: new pipette selection dropdown ends the modal when selected, working on this separately

  • clicking [settings] for an attached pipettes pops up pipette config modal. (please confirm settings are still updatable.

@Laura-Danielle
Copy link
Contributor

Found another issue (separate maybe?):

When I am in the attach pipettes flow and I have detached one pipette in order to attach another, if I select which pipette to attach the modal disappears completely and I cannot finish the flow.

Copy link
Contributor

@Laura-Danielle Laura-Danielle left a comment

Choose a reason for hiding this comment

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

💃

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.

🍾

@@ -53,14 +53,14 @@ function AttachedPipettesCard(props: Props) {
<CardContentFlex>
<InstrumentInfo
mount="left"
name={props.robot.name}
robotName={props.robot.name}
{...props.left}
Copy link
Contributor

@mcous mcous May 21, 2019

Choose a reason for hiding this comment

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

If I remember correctly, this spread is only to grab the pipette model, so it could be changed to:

model={props.left?.model}

{...props.left}
onChangeClick={props.clearMove}
showSettings={props.showLeftSettings}
/>
<InstrumentInfo
mount="right"
name={props.robot.name}
robotName={props.robot.name}
{...props.right}
Copy link
Contributor

Choose a reason for hiding this comment

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

Same change as above:

model={props.right?.model}

@Kadee80 Kadee80 merged commit 2419110 into edge May 21, 2019
@Kadee80 Kadee80 deleted the app_fix-attached-pipettes-card branch May 21, 2019 21:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
app Affects the `app` project bug fix PR fixes a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants