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

refactor(app): Consolidate /pipettes calls into common API #2262

Merged
merged 2 commits into from
Sep 14, 2018

Conversation

Kadee80
Copy link
Contributor

@Kadee80 Kadee80 commented Sep 13, 2018

overview

Small refactor PR in to make future API work around connectivity state easier with regards to #2100.

Since pipettes is a one off case with refresh it does not implement buildRequestMaker.

This pr touches motors as well since they depend on pipettes. Theres a TODO about storing motors in state that might require further discussion.

changelog

  • refactor(app): Consolidate /pipettes calls into common API

review requests

tests pass and everything looks cool on VS but please test all pipette related pages on an actual robot with pipettes to ensure everything still works!

  • InstrumentSettings page pipettes card
  • FileInfo page pipettes section
  • Pipette calibration pages

@Kadee80 Kadee80 self-assigned this Sep 13, 2018
@Kadee80 Kadee80 requested a review from mcous September 13, 2018 18:00
@@ -26,8 +26,7 @@ type SettingsRequest = ?{id: Id, value: Value}

type SettingsResponse = {settings: Array<Setting>}

export type SettingsAction =
ApiAction<'settings', SettingsRequest, SettingsResponse>
export type SettingsAction = ApiAction<'settings', SettingsRequest, SettingsResponse>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry about this, really a whitespace issue that snuck in. LMK

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.

☕️

(Haven't tested on a real robot yet, probably hold off on merge until we get a real robot test in)

@mcous
Copy link
Contributor

mcous commented Sep 14, 2018

  • InstrumentSettings page pipettes card
  • FileInfo page pipettes section
  • Pipette calibration pages
  • Intercom update with pipettes

@Kadee80 Kadee80 merged commit 54f679f into edge Sep 14, 2018
@Kadee80 Kadee80 deleted the app_refactor-pipettes-state branch September 14, 2018 17:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
app Affects the `app` project refactor small
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants