feat(app): Add attached modules card UI to instrument settings page #1854
Conversation
Kadee80
added
feature
| return ( | ||
| <React.Fragment> | ||
| {props.modules.map((mod) => ( | ||
| <ModuleItem {...mod} /> |
There was a problem hiding this comment.
yes good catch. saw the warning and then forgot to slug it in
|
|
||
| // TODO (ka 2018-7-10): replace with design assets onces available | ||
| const MODULE_IMGS = { | ||
| 'Temperature Module': 'http://via.placeholder.com/100x75', |
There was a problem hiding this comment.
What, no http://placekitten.com?
There was a problem hiding this comment.
I will placecage in a pinch
| return ( | ||
| <div className={styles.module_update}> | ||
| <OutlineButton | ||
| disabled={!!availableUpdate} |
There was a problem hiding this comment.
This disabled logic should be switched
There was a problem hiding this comment.
good catch reversing now
Codecov Report
@@ Coverage Diff @@
## edge #1854 +/- ##
==========================================
+ Coverage 33.76% 33.85% +0.09%
==========================================
Files 391 396 +5
Lines 6380 6428 +48
==========================================
+ Hits 2154 2176 +22
- Misses 4226 4252 +26
Continue to review full report at Codecov.
|
| fetchModules?: () => mixed | ||
| } | ||
|
|
||
| type Props = SP | ||
|
|
||
| const TITLE = 'Modules' | ||
|
|
||
| const MODULE_DATA = [ |
There was a problem hiding this comment.
Could this name forecast that it is temporary stub data more? e.g. STUBBED_MODULE_DATA. especially next to TITLE it looks like a permanent constant
| export default function ModuleItem (props: Module) { | ||
| return ( | ||
| <div className={styles.module_item}> | ||
| <ModuleImage {...props}/> |
There was a problem hiding this comment.
Is it true that these child components are receiving a lot of extra props because of these spreads? Is there any reason you're not explicitly passing down the props that they'll need. e.g. <ModuleUpdate availableUpdate={props.availableUpdate} />
There was a problem hiding this comment.
happy to be more specific. will change now
| import styles from './styles.css' | ||
|
|
||
| type Props = { | ||
| availableUpdate?: ?string |
There was a problem hiding this comment.
I don't see availableUpdate in the stubbed modules. Is it a version string in the case that there is an available update and undefined if not? If that's the case, could it be called availableUpdateVersion?
There was a problem hiding this comment.
Currently, everything is just stubbed out. Not sure where the firmware update will come from yet. In robot it was a ?string with the version if a new one is available
There was a problem hiding this comment.
availableUpdate will likely come from a selector based on whatever firmware files we have bundled in that version of the app
| import styles from './styles.css' | ||
|
|
||
| type Props = { | ||
| availableUpdate?: ?string |
There was a problem hiding this comment.
availableUpdate will likely come from a selector based on whatever firmware files we have bundled in that version of the app
overview
This PR closes #1735 (assuming wiring up to API endpoint is its own ticket once the endpoint exists)
changelog
review requests
Keeping this as a WIP while waiting for pending modules image assets. ModuleItems currently just uses a placehold.it image. Once we have those, another styling pass before ready for review.
I pulled
ModuleItemand its atoms along withNoModulesMessageinto a/ModuleItemdirectory because instrumentSettings was getting a little cluttered.Also worth noting, since the endpoint does not exist and is not wired mapStateToProps is rendering mock data.
Modules is still behind a feature flag so run
OT_APP_MODULES=1 make devto view it inInstrumentSettings