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): Add and implement module selectors in calibration #1895

Merged
merged 2 commits into from
Jul 18, 2018

Conversation

Kadee80
Copy link
Contributor

@Kadee80 Kadee80 commented Jul 18, 2018

overview

This PR addresses #1737 by adding and implementing module selectors and actions in Calibrate/Labware. It also introduces a RefreshWrapper for pages which is implemented on page mount in order to fetchPipettes and fetchModules from the api.

changelog

  • feat(app): Add and implement module selectors in calibration
  • add RefreshWrapper component and implement on calibration pages

review requests

If you'd like to see a populated modulesBySlot comment out line 343 and uncomment lines 345-351 in app/src/robot/api-client/client.js. *Please note tests wont pass with this uncommented.

  • fetchPipettes is called on mount for calibrate/pipettes
  • fetchModules is called on mount for calibrate/labware
  • api[RobotName].api.modules exists and response = { modules: [ ] }

@Kadee80 Kadee80 added feature Ticket is a feature request / PR introduces a feature app Affects the `app` project WIP medium labels Jul 18, 2018
@Kadee80 Kadee80 added this to the Modules milestone Jul 18, 2018
@Kadee80 Kadee80 self-assigned this Jul 18, 2018
@Kadee80 Kadee80 requested a review from mcous July 18, 2018 18:27
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.

Got some tweaks we can make to RefreshWrapper but the stuff I mentioned isn't being used so it's not big deal at the moment

🌌


type Props = {
watch?: string,
refreshing?: boolean,
Copy link
Contributor

Choose a reason for hiding this comment

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

Prop not used

Copy link
Contributor Author

Choose a reason for hiding this comment

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

not used... yet?

import * as React from 'react'

type Props = {
watch?: string,
Copy link
Contributor

Choose a reason for hiding this comment

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

Adding watch to RefreshCard was a mistake on my part. React's built-in key prop was what I should've used (and what we should use here)

this.props.refresh()
}

componentDidUpdate (prevProps: Props) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can remove this, provided we use key instead of watch

@@ -78,15 +76,3 @@ function mapStateToProps (state: State, ownProps: OP): SP {

return {...NAV_ITEM_BY_NAME[name], _robot}
}

function mergeProps (stateProps: SP, dispatchProps: DP, ownProps: OP): Props {
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@Kadee80 Kadee80 merged commit 2cf1b4d into edge Jul 18, 2018
@Kadee80 Kadee80 deleted the app_module-missing-state branch July 18, 2018 20:02
@mcous mcous mentioned this pull request Aug 3, 2018
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 medium
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants