-
Notifications
You must be signed in to change notification settings - Fork 3
Connect light sensors #217
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
Conversation
| "build": "webpack --config webpack.production.js", | ||
| "i18n:manage": "node src/translations/runner.js", | ||
| "test": "jest --coverage --color", | ||
| "test:interactive": "jest --watch --coverage --color", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This tool is ... interesting. I'm not sure if it made things faster for me or not, but it's worth trying out!
| classes, | ||
| }) => ( | ||
| <> | ||
| <ExpansionPanelDetails> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't love that we assume that this component is used inside an ExpansionPanel. However, if you don't put things inside their own ExpansionPanelDetails, it tries to smash them together side-by-side for some reason. I thought this solution would work ok for now.
| <FormattedMessage | ||
| id="app.mission_control.buttons" | ||
| description="Describes the buttons sensor section" | ||
| defaultMessage="Buttons" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: I added this label here to show what it will look like, but I did not yet change the functionality of these indicators. They are still hooked up to change with the light sensor values. We'll need to change them to show "button has been pressed" in the next PR.
| @@ -0,0 +1,47 @@ | |||
| import React from 'react'; | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The name of this file is inconsistent with the module it is testing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
| import NumericSensorReadout from '../NumericSensorReadout'; | ||
|
|
||
| describe('The NumericSensorReadout component', () => { | ||
| test('renders values on the page with no erros when values present', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
errors
cabarnes
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
This PR
I mostly wanted to get the light sensors wired up, but I went ahead and also did the battery voltage as an example of how to use the NumericalSensorReadout component in a different way.