refactor(app): wire up heater shaker wizard TestShake page #9833

sakibh · 2022-03-30T17:09:10Z

Overview

This PR wires up the buttons and input for the testshake page of the heater shaker wizard. closes #9613

Changelog

Wire up TestShake.tsx
Add test coverage

Review requests

Check that the design matches the Figma and the buttons/input are wired correctly.

Note: Still need to dynamically render banner text: if there is a protocol the plate name should come from the labware definition.

Risk assessment

low

This PR wires up the buttons and input for the testshake page of the heater shaker wizard. closes #9613

codecov · 2022-03-30T17:15:23Z

Codecov Report

Merging #9833 (dc61f24) into edge (4a85bbd) will increase coverage by 0.00%.
The diff coverage is 76.92%.

@@           Coverage Diff           @@
##             edge    #9833   +/-   ##
=======================================
  Coverage   75.02%   75.02%           
=======================================
  Files        2009     2009           
  Lines       53326    53357   +31     
  Branches     5160     5172   +12     
=======================================
+ Hits        40007    40031   +24     
- Misses      12297    12303    +6     
- Partials     1022     1023    +1

Flag	Coverage Δ
app	`71.64% <76.92%> (+0.01%)`	⬆️

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

Impacted Files	Coverage Δ
app/src/atoms/InputField/index.tsx	`81.25% <ø> (ø)`
...organisms/Devices/ModuleCard/TestShakeSlideout.tsx	`75.00% <ø> (ø)`
...tup/HeaterShakerSetupWizard/HeaterShakerBanner.tsx	`57.14% <0.00%> (ø)`
...isms/Devices/ModuleCard/HeaterShakerModuleData.tsx	`53.33% <50.00%> (-3.81%)`	⬇️
...organisms/Devices/HeaterShakerWizard/TestShake.tsx	`83.33% <88.00%> (+11.90%)`	⬆️
...src/organisms/Devices/HeaterShakerWizard/index.tsx	`84.21% <100.00%> (+2.15%)`	⬆️

jerader

Overall looking pretty good! great job! Left a few comments.

Overall, I think we should be consistent on what idle_unknown means for the latch status. I remember discussing in the past that it means we should default to the latch being opened since we always wants to make sure the latch is closed before proceeding to use it.

jerader · 2022-03-30T19:50:20Z

app/src/organisms/Devices/ModuleCard/HeaterShakerModuleData.tsx

+                <Icon
+                  name="closed-locked"
+                  data-testid="HeaterShakerModuleData_latch_lock"
+                  size={'1rem'}


should we make the 1 rem size a constant somewhere?

Looks like we do have layout constants: components/src/styles/layout.ts.

jerader · 2022-03-30T19:54:37Z

app/src/organisms/Devices/HeaterShakerWizard/TestShake.tsx

+  const isLatchOpen =
+    module.data.labwareLatchStatus === 'idle_open' ||
+    module.data.labwareLatchStatus === 'opening'
+


I thought we also wanted idle_unknown status to default to the latch being opened because we want to always ensure the latch is closed before the heater shaker is shaking/heating

jerader · 2022-03-30T19:55:47Z

app/src/organisms/Devices/HeaterShakerWizard/TestShake.tsx

+    module.data.labwareLatchStatus === 'idle_open' ||
+    module.data.labwareLatchStatus === 'opening'
+
+  const setLatchCommand:


you can use the useLatchCommand hook for this!

jerader · 2022-03-30T19:58:30Z

app/src/organisms/Devices/HeaterShakerWizard/__tests__/TestShake.test.tsx

+  it('entering an input for shake speed and clicking start should begin shaking', () => {
+    props = {


You can add another test case here for when clicking on the button when the it is shaking will stop the shake.

jerader · 2022-03-30T20:00:53Z

app/src/organisms/Devices/ModuleCard/HeaterShakerModuleData.tsx

+  heaterStatus: TemperatureStatus
+  shakerStatus: SpeedStatus
+  latchStatus: LatchStatus


since these all come from module.data, you could decrease the number of props and just have it take in module.data - looks like HeaterShakerData is an importable type.

We can totally do that. I decided to stick with this since other modules with similar components do it this way and it describes the component better.

sakibh · 2022-04-05T19:13:07Z

app/src/atoms/InputField/index.tsx

+    input[type='number']::-webkit-inner-spin-button,
+    input[type='number']::-webkit-outer-spin-button {
+      -webkit-appearance: none;
+      margin: 0;
+    }


This additional css hides the up/down arrows when the InputField component strictly takes in numbers.

why do we want to hide arrows when the input field only takes numbers? wouldn't we want the opposite?

also do we need two lines with input[type='number']::-webkit-inner-spin-button?

It looks like we need to put both for the inner and one for the outer to hide the arrows.

shlokamin

code LGTM! once we get specs for disabling module card CTAs when there is an active run we'll have to come back and conditionally use createLiveCommand or createCommand depending on whether or not there is an active run, but we can worry about that later!

shlokamin · 2022-04-05T20:09:08Z

app/src/atoms/InputField/index.tsx

+    input[type='number']::-webkit-inner-spin-button,
+    input[type='number']::-webkit-outer-spin-button {
+      -webkit-appearance: none;
+      margin: 0;
+    }


why do we want to hide arrows when the input field only takes numbers? wouldn't we want the opposite?

also do we need two lines with input[type='number']::-webkit-inner-spin-button?

jerader

LGTM!

* refactor(app): wire up heater shaker wizard TestShake page This PR wires up the buttons and input for the testshake page of the heater shaker wizard. closes #9613

sakibh added 3 commits March 29, 2022 20:55

wire up test shake page

d283b93

Merge branch 'edge' into app_hs-wizard-test-shake-wire-up

4baf61f

refactor(app): wire up heater shaker wizard TestShake page

c5e9055

This PR wires up the buttons and input for the testshake page of the heater shaker wizard. closes #9613

sakibh requested review from b-cooper, shlokamin and jerader March 30, 2022 17:09

sakibh requested a review from a team as a code owner March 30, 2022 17:09

sakibh self-assigned this Mar 30, 2022

sakibh removed the request for review from a team March 30, 2022 17:09

jerader requested changes Mar 30, 2022

View reviewed changes

sakibh added 5 commits April 1, 2022 13:54

fix conflict

13eabf6

address PR comments

ef6bc31

fix merge conflict

8f7911d

update tests and inputfield component

97d77e5

add size constant

dc61f24

sakibh requested a review from jerader April 5, 2022 19:12

sakibh commented Apr 5, 2022

View reviewed changes

shlokamin approved these changes Apr 5, 2022

View reviewed changes

jerader approved these changes Apr 7, 2022

View reviewed changes

sakibh merged commit 1f34210 into edge Apr 7, 2022

sakibh deleted the app_hs-wizard-test-shake-wire-up branch April 7, 2022 13:24

sakibh mentioned this pull request May 9, 2022

Wire up test shake inputs #9700

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(app): wire up heater shaker wizard TestShake page #9833

refactor(app): wire up heater shaker wizard TestShake page #9833

sakibh commented Mar 30, 2022 •

edited

Loading

codecov bot commented Mar 30, 2022 •

edited

Loading

jerader left a comment

jerader Mar 30, 2022

sakibh Apr 5, 2022 •

edited

Loading

jerader Mar 30, 2022

jerader Mar 30, 2022

jerader Mar 30, 2022

jerader Mar 30, 2022

sakibh Apr 5, 2022

sakibh Apr 5, 2022

shlokamin Apr 5, 2022

sakibh Apr 7, 2022

shlokamin left a comment

shlokamin Apr 5, 2022

jerader left a comment

		it('entering an input for shake speed and clicking start should begin shaking', () => {
		props = {

refactor(app): wire up heater shaker wizard TestShake page #9833

refactor(app): wire up heater shaker wizard TestShake page #9833

Conversation

sakibh commented Mar 30, 2022 • edited Loading

Overview

Changelog

Review requests

Risk assessment

codecov bot commented Mar 30, 2022 • edited Loading

Codecov Report

jerader left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sakibh Apr 5, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shlokamin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jerader left a comment

Choose a reason for hiding this comment

sakibh commented Mar 30, 2022 •

edited

Loading

codecov bot commented Mar 30, 2022 •

edited

Loading

sakibh Apr 5, 2022 •

edited

Loading