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): wire components to new state.modules tree #4615

Merged
merged 1 commit into from
Dec 16, 2019

Conversation

mcous
Copy link
Contributor

@mcous mcous commented Dec 16, 2019

overview

This PR wires up the work of #4599 to the necessary components, and does a little associated cleanup work.

It also unblocks #4576

changelog

  • refactor(app): wire components to new state.modules tree

cleanup details

  • Many components that used to need a full Robot can now make do with a simple robotName: string, because the new modules actions only need a robotName (IP address is looked up from state by the epic)
  • Switched pretty much every component I touched to a named exports based on newer conventions (see Decision Log in our FE cookbook)
  • Moved useSendModuleCommand hook to src/modules/hooks and added tests
  • Removed anything deprecated and unused in app/src/robot-api
    • Moved everything deprecated but still used to app/src/robot-pi/deprecated.js

review requests

@mcous mcous added app Affects the `app` project ready for review refactor labels Dec 16, 2019
@mcous mcous requested review from b-cooper and a team December 16, 2019 17:11
@codecov
Copy link

codecov bot commented Dec 16, 2019

Codecov Report

Merging #4615 into edge will increase coverage by 0.11%.
The diff coverage is 38.46%.

Impacted file tree graph

@@            Coverage Diff             @@
##             edge    #4615      +/-   ##
==========================================
+ Coverage   57.85%   57.97%   +0.11%     
==========================================
  Files         935      933       -2     
  Lines       26078    25995      -83     
==========================================
- Hits        15088    15071      -17     
+ Misses      10990    10924      -66
Impacted Files Coverage Δ
app/src/components/ModuleItem/ModuleImage.js 0% <ø> (ø) ⬆️
app/src/robot-api/operators.js 100% <ø> (ø) ⬆️
app/src/modules/actions.js 100% <ø> (ø) ⬆️
...src/components/ModuleLiveStatusCards/StatusItem.js 0% <ø> (ø) ⬆️
app/src/shell/buildroot/reducer.js 85.36% <ø> (ø) ⬆️
app/src/shell/buildroot/update-epics.js 77.68% <ø> (ø) ⬆️
app/src/components/RunPanel/index.js 0% <ø> (ø) ⬆️
app/src/components/InstrumentSettings/index.js 0% <ø> (ø) ⬆️
app/src/robot-settings/epic.js 100% <ø> (ø) ⬆️
app/src/components/ModuleItem/NoModulesMessage.js 0% <ø> (ø) ⬆️
... and 30 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 9f6f7db...40fb6a8. Read the comment docs.

Copy link
Contributor

@b-cooper b-cooper left a comment

Choose a reason for hiding this comment

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

Code looks good 🚡. Ot of curiosity, what is your issue with default exports for leaf components files?

@b-cooper
Copy link
Contributor

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