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): Fix modules not populating the modules card #3278

Merged
merged 2 commits into from
Mar 29, 2019

Conversation

Laura-Danielle
Copy link
Contributor

overview

Logic was introduced into edge which made the modules (temperature + magnetic) not populate the modules card in the app even though the endpoint was returning correctly formatted data. We will need to do another alpha release once this is merged.

changelog

  • Remove logic checking for thermocycler flag
  • Fix a typo in models.py

review requests

Please test this branch on your robot

@Laura-Danielle Laura-Danielle added app Affects the `app` project api Affects the `api` project ready for review labels Mar 29, 2019
@Laura-Danielle Laura-Danielle requested review from a team March 29, 2019 16:09
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.

💸

@@ -9,8 +9,8 @@ type Props = {
}

export default function ModulesCardContents (props: Props) {
const {modules, showThermo} = props
if (!modules || !modules[0] || !showThermo) return <NoModulesMessage />
const {modules} = props
Copy link
Contributor

@mcous mcous Mar 29, 2019

Choose a reason for hiding this comment

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

To preserve intended functionality, could we consider this change instead?

// ...
import filter from 'lodash/filter'
// ...
export default function ModulesCardContents (props: Props) {
  const {showThermo} = props
  const modules = filter(props.modules, m => {
    return showThermo || m.name !== 'thermocycler'
  })

  if (modules.length === 0) return <NoModulesMessage />

  return (
    <React.Fragment>
      {modules.map((mod, index) => (
        <ModuleItem module={mod} key={index} />
      ))}
    </React.Fragment>
  )
}

@codecov
Copy link

codecov bot commented Mar 29, 2019

Codecov Report

Merging #3278 into edge will decrease coverage by <.01%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##             edge    #3278      +/-   ##
==========================================
- Coverage   53.58%   53.57%   -0.01%     
==========================================
  Files         711      711              
  Lines       20739    20742       +3     
==========================================
  Hits        11112    11112              
- Misses       9627     9630       +3
Impacted Files Coverage Δ
api/src/opentrons/api/models.py 84.09% <0%> (ø) ⬆️
...mponents/InstrumentSettings/ModulesCardContents.js 0% <0%> (ø) ⬆️

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 e4ca4ff...7458b93. Read the comment docs.

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.

Appreciate the changes! 🍓

@sfoster1 sfoster1 added the small label Mar 29, 2019
@Laura-Danielle Laura-Danielle merged commit 1fd936d into edge Mar 29, 2019
@Laura-Danielle Laura-Danielle deleted the fix_module_card_display branch March 29, 2019 16:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api Affects the `api` project app Affects the `app` project small
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants