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): Allow valid pipette+ model names for display images #3413

Merged
merged 2 commits into from
May 10, 2019

Conversation

Laura-Danielle
Copy link
Contributor

@Laura-Danielle Laura-Danielle commented May 3, 2019

overview

Closes #3340. Due to the format of the pipette+ model names, images were not automatically being displayed on the instruments card.

changelog

  • Change REGEX to determine which images propogate in Run App on attached instruments card

review requests

@mcous I see that you made a note that volumes and channels should come from API, should that be the correct implementation here or is changing the expression sufficient?

@Laura-Danielle Laura-Danielle added app Affects the `app` project ready for review labels May 3, 2019
@Laura-Danielle Laura-Danielle requested review from mcous and Kadee80 May 3, 2019 18:17
@@ -21,7 +21,7 @@ type Props = {
}

// TODO(mc, 2018-03-30): volume and channels should come from the API
const RE_CHANNELS = /p\d+_(single|multi)/
const RE_CHANNELS = /p(\d+|\+\d+)_(single|multi)/
Copy link
Contributor

Choose a reason for hiding this comment

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

Given the TODO above, could we address the underlying problem rather than continue to rely on this regex?

// ...
import { getPipetteModelSpecs } from '@opentrons/shared-data'
// ...

export default function PipetteInfo(props: Props) {
  const { mount, model, name, onChangeClick, showSettings } = props
  const label = LABEL_BY_MOUNT[mount]
  const pipette = model ? getPipetteModelSpecs(model) : null
  const channels = pipette?.channels
  // ...

  return (
    // ...
      <div className={styles.image}>
        {channels && (
          <InstrumentDiagram
            channels={channels}
            className={styles.pipette_diagram}
          />
        )}
      </div>
    // ...
  )

@codecov
Copy link

codecov bot commented May 9, 2019

Codecov Report

Merging #3413 into edge will decrease coverage by 1%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##             edge    #3413      +/-   ##
==========================================
- Coverage   52.59%   51.58%   -1.01%     
==========================================
  Files         775      800      +25     
  Lines       22980    23711     +731     
==========================================
+ Hits        12086    12232     +146     
- Misses      10894    11479     +585
Impacted Files Coverage Δ
...rc/components/InstrumentSettings/InstrumentInfo.js 0% <0%> (ø) ⬆️
...ry/src/components/LabwareDetails/WellDimensions.js 14.28% <0%> (-2.39%) ⬇️
...-library/src/components/LabwareList/LabwareCard.js 3.84% <0%> (-0.16%) ⬇️
opentrons/legacy_api/robot/mover.py 88.88% <0%> (-0.13%) ⬇️
...c/components/ModuleControls/TemperatureControls.js 0% <0%> (ø) ⬆️
app/src/index.js 0% <0%> (ø) ⬆️
app/src/components/ConnectModulesModal/index.js 0% <0%> (ø) ⬆️
update-server/otupdate/buildroot/__main__.py 0% <0%> (ø) ⬆️
app/src/reducer.js 0% <0%> (ø) ⬆️
app/src/http-api-client/index.js 100% <0%> (ø) ⬆️
... and 39 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 e5e73bc...dbf44bd. Read the comment docs.

@Laura-Danielle Laura-Danielle requested a review from mcous May 9, 2019 16:13
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.

💸

@Laura-Danielle Laura-Danielle merged commit 1f77a08 into edge May 10, 2019
@Laura-Danielle Laura-Danielle deleted the app_display_ppp_images branch May 10, 2019 19:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
app Affects the `app` project
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Pipette+: App Should Correctly Display pipette images for new models
3 participants