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): move robot lights controls to state.robotControls #4631

Merged
merged 3 commits into from
Dec 18, 2019

Conversation

mcous
Copy link
Contributor

@mcous mcous commented Dec 17, 2019

overview

This PR follows up on the refactor work completed in #4440, #4477, #4544, #4599, and #4615. The endpoints being moved today are:

  • GET /robot/lights
  • POST /robot/lights

changelog

  • Moved robot lights controls to app/src/robot-controls
    • Added 6 actions (fetchLights + success / failure, updateLights + succcess / failure)
    • Added state.robotControls reducer
    • Added 1 selector (getLightsOn)
    • Added 2 epics (fetchLightsEpic, updateLightsEpic)
    • Added tests for all of the above
  • Added the following tests to RobotControlsCard:
    • Calls fetchLights on mount
    • Calls updateLights on lights toggle click
    • Calls restartRobot on restart button click
    • DC, home, and restart buttons are disabled if not connectable
    • DC, home, and restart buttons are disabled if not connected
    • DC, home, and restart buttons are disabled if connected but protocol is running

Important to note: Previously, the deck calibration button was not disabled if you weren't "connected" to the robot. Given that the "Home" and "Restart" buttons were disabled in this instance, the DC button seemed like an oversight. This PR now guards the DC button behind the same logic as "Home" and "Restart"

review requests

All changes are in the "Robot Controls" card of the robot settings page

  • Lights toggle shows up properly on navigation to robot (with lights already on and off)
    • There might be a slight delay as GET /robot/lights is called
  • Lights toggle is able to toggle lights
  • "Home" button homes the robot if connected and no protocol running
  • "Restart" button restarts the robot if connected and no protocol running
  • "Calibrate" button starts deck calibration the robot if connected and no protocol running
  • Disabled cases listed above in added tests all work

@mcous mcous added app Affects the `app` project ready for review refactor labels Dec 17, 2019
@mcous mcous requested a review from a team December 17, 2019 22:59
@codecov
Copy link

codecov bot commented Dec 17, 2019

Codecov Report

Merging #4631 into edge will decrease coverage by 0.14%.
The diff coverage is 93.44%.

Impacted file tree graph

@@            Coverage Diff             @@
##             edge    #4631      +/-   ##
==========================================
- Coverage   57.97%   57.83%   -0.15%     
==========================================
  Files         938      948      +10     
  Lines       26046    26492     +446     
==========================================
+ Hits        15100    15321     +221     
- Misses      10946    11171     +225
Impacted Files Coverage Δ
app/src/http-api-client/index.js 100% <ø> (ø) ⬆️
app/src/components/RobotSettings/index.js 0% <ø> (ø) ⬆️
app/src/reducer.js 0% <ø> (ø) ⬆️
app/src/robot-controls/constants.js 100% <100%> (ø)
app/src/robot-controls/epic/updateLightsEpic.js 100% <100%> (ø)
app/src/robot-controls/selectors.js 100% <100%> (ø)
app/src/robot-controls/epic/index.js 100% <100%> (ø)
app/src/robot-controls/actions.js 100% <100%> (ø)
app/src/http-api-client/robot.js 96.72% <100%> (-0.69%) ⬇️
app/src/robot-controls/epic/fetchLightsEpic.js 100% <100%> (ø)
... and 27 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 f869c4c...2486ee5. Read the comment docs.

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.

💡 ⚡️

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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants