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): guide the user through leveling gen2 multis #5348

Merged
merged 12 commits into from
Apr 20, 2020

Conversation

sfoster1
Copy link
Member

@sfoster1 sfoster1 commented Apr 1, 2020

This new flow added in to change pipette guides the user through making
sure their gen2 multi pipettes are level, using the extension block and
manually pulling the pipettes against it.

Closes #5344

Design reference:
new text on pre-attach screen: https://app.zeplin.io/project/5aa97729db58a2192f10d0d6/screen/5e839ed0f792546d4c858d30
leveling screen: https://app.zeplin.io/project/5aa97729db58a2192f10d0d6/screen/5e839ed07319a9140ea11305

Status

Ready for test and review!

Risk Assessment

Really low, this is just adding a screen to the change pipette flow and slightly modifying the text in another

This new flow added in to change pipette guides the user through making
sure their gen2 multi pipettes are level, using the extension block and
manually pulling the pipettes against it.

Closes #5344
@sfoster1 sfoster1 added feature Ticket is a feature request / PR introduces a feature app Affects the `app` project labels Apr 1, 2020
@sfoster1 sfoster1 requested review from a team April 1, 2020 17:08
@codecov
Copy link

codecov bot commented Apr 1, 2020

Codecov Report

Merging #5348 into edge will increase coverage by 3.02%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             edge    #5348      +/-   ##
==========================================
+ Coverage   63.44%   66.47%   +3.02%     
==========================================
  Files        1076     1097      +21     
  Lines       30649    33938    +3289     
==========================================
+ Hits        19445    22559    +3114     
- Misses      11204    11379     +175     
Impacted Files Coverage Δ
app/src/components/ChangePipette/Instructions.js 0.00% <0.00%> (ø)
app/src/components/ChangePipette/LevelPipette.js 0.00% <0.00%> (ø)
app/src/components/ChangePipette/index.js 0.00% <0.00%> (ø)
shared-data/js/pipettes.js 94.44% <0.00%> (-5.56%) ⬇️
...er/src/components/StepEditForm/forms/MagnetForm.js 72.72% <0.00%> (-7.28%) ⬇️
...er/src/components/modals/EditModulesModal/index.js 91.11% <0.00%> (-6.51%) ⬇️
protocol-designer/src/load-file/migration/1_1_0.js 80.95% <0.00%> (-3.11%) ⬇️
...designer/src/components/FileSidebar/FileSidebar.js 76.54% <0.00%> (-2.53%) ⬇️
...col-designer/src/components/DeckSetup/ModuleTag.js 80.00% <0.00%> (-1.82%) ⬇️
protocol-designer/src/feature-flags/reducers.js 38.88% <0.00%> (-1.12%) ⬇️
... and 95 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 8235e71...10a90c7. Read the comment docs.

)
}

function ExitButton(props: Props) {
Copy link
Contributor

@b-cooper b-cooper Apr 2, 2020

Choose a reason for hiding this comment

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

Nit: because this component is so shallow, I'd probably place the instance of PrimaryButton directly in the LevelPipette component for clarity in this file. This kind of abstraction feels unnecessary. You could probably make the same argument for the Status component. It's not really adding branching logic, just a haven for some variable declarations.

@sfoster1 sfoster1 marked this pull request as ready for review April 13, 2020 21:22
@sfoster1 sfoster1 requested review from pantslakz and a team April 13, 2020 21:22
Copy link
Contributor

@b-cooper b-cooper 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 to merge, don't have a GEN2 multi to test with unfortunately.

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.

One change request for video sizing. Otherwise this code looks pretty clean. Unable to test because I don't have a robot and left some nits because I was already commenting

app/src/components/ChangePipette/LevelPipette.js Outdated Show resolved Hide resolved
app/src/components/ChangePipette/styles.css Outdated Show resolved Hide resolved
app/src/components/ChangePipette/styles.css Outdated Show resolved Hide resolved
app/src/components/ChangePipette/LevelPipette.js Outdated Show resolved Hide resolved
Copy link

@pantslakz pantslakz left a comment

Choose a reason for hiding this comment

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

🔥

@sfoster1 sfoster1 merged commit 185d0ad into edge Apr 20, 2020
@sfoster1 sfoster1 deleted the app_gen2-multi-leveling branch April 20, 2020 14:38
IanLondon pushed a commit that referenced this pull request Apr 22, 2020
Adds a new flow to attach pipette that guides the user through making
sure their gen2 multi pipettes are level, using the extension block and
manually pulling the pipettes against it. It involves four new videos and therefore also adds webpack support for .webm files.

Closes #5344
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.

Gen2 Multi Levelling: Add flows
4 participants