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): Remove incorrect data removal warning from change pipette #3942

Merged
merged 1 commit into from
Aug 26, 2019

Conversation

mcous
Copy link
Contributor

@mcous mcous commented Aug 26, 2019

overview

Tiny bug + fix PR to address some feedback about confusing UX surfaced from a user. When you go to change a pipette, it presents the following warning:

Detaching a pipette will also clear its related calibration data

This is not accurate. The robot will take no action to clear any sort of settings about this pipette. Best guess is that this message was trying to refer to the fact that you'll probably need to re-do any run-time calibration (tip probe, labware position calibration) you've started for the protocol you have uploaded, if any, but:

  • That's got nothing to do with robot data, just that the RPC API isn't good enough for us to have a robust app-side state for runtime calibration
  • That message isn't clear if runtime calibration is, in fact, what the message is trying to reference
  • Starting runtime calibration with the wrong pipette, changing it, and then have to restart runtime calibration is not a surprising experience, so (IMO) we don't need extra copy

changelog

  • fix(app): Remove incorrect data removal warning from change pipette

review requests

  • Copy removal makes sense

@mcous mcous added bug app Affects the `app` project ready for review fix PR fixes a bug labels Aug 26, 2019
@mcous mcous requested review from pantslakz and a team August 26, 2019 19:57
Copy link
Contributor

@btmorr btmorr left a comment

Choose a reason for hiding this comment

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

Makes sense

Copy link
Contributor

@Kadee80 Kadee80 left a comment

Choose a reason for hiding this comment

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

Looks good. Tested on robot.

@codecov
Copy link

codecov bot commented Aug 26, 2019

Codecov Report

Merging #3942 into edge will increase coverage by 1.14%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##             edge    #3942      +/-   ##
==========================================
+ Coverage   57.54%   58.68%   +1.14%     
==========================================
  Files         827      827              
  Lines       23435    24264     +829     
==========================================
+ Hits        13486    14240     +754     
- Misses       9949    10024      +75
Impacted Files Coverage Δ
app/src/components/ChangePipette/index.js 0% <0%> (ø) ⬆️
opentrons/util/calibration_functions.py 100% <0%> (ø) ⬆️
opentrons/hardware_control/modules/tempdeck.py 95.97% <0%> (+0.32%) ⬆️
opentrons/hardware_control/modules/magdeck.py 90.98% <0%> (+0.87%) ⬆️
opentrons/protocol_api/execute.py 72.57% <0%> (+1.63%) ⬆️
opentrons/hardware_control/adapters.py 85.21% <0%> (+2.02%) ⬆️
update-server/otupdate/migration/file_actions.py 87.85% <0%> (+2.4%) ⬆️
opentrons/hardware_control/modules/thermocycler.py 83.13% <0%> (+2.71%) ⬆️
opentrons/protocols/execute_v1.py 77.27% <0%> (+3.54%) ⬆️
opentrons/drivers/smoothie_drivers/driver_3_0.py 81.13% <0%> (+4%) ⬆️
... and 4 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 30cdf17...f1b4e8c. Read the comment docs.

@mcous mcous merged commit 27b315c into edge Aug 26, 2019
@mcous mcous deleted the app_remove-bogus-pipette-change-warning branch August 26, 2019 20:13
@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 bug fix PR fixes a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants