-
Notifications
You must be signed in to change notification settings - Fork 6
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
TMA Implementation #352
TMA Implementation #352
Conversation
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.
Great job here Fernando! I left some comments regarding some details, there rest looks very good
love/src/Config.js
Outdated
3: 'HHD', | ||
}; | ||
|
||
// TODO: Pending Style |
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 is still pending? If so, what is missing?
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.
Yes, need validation about the colors used for the states.
@MiaRoseElbo
connected: summaryData['event-MTMount-0-connected'] | ||
? summaryData['event-MTMount-0-connected'][0].command.value | ||
: false, | ||
balancing: summaryData['event-MTMount-0-commander'] |
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.
Here should be event-MTMount-0-balanceSystemState
instead of event-MTMount-0-commander
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.
thanks
flex-direction: column; | ||
padding: 1em; | ||
border-radius: 5px; | ||
/* background: #132631; svg color, slightly better contrast with background */ |
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.
Also this one?
const equivalentAzimuthActual = this.closestEquivalentAngle(this.prevAzimuthActual, this.props.azimuthActualPosition); | ||
const equivalentAzimuthDemand = this.closestEquivalentAngle(this.prevAzimuthDemand, this.props.azimuthDemandPosition); |
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.
And here you should call this.state.prevAzimuthActual
and this.state.prevAzimuthDemand
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.
Ok, its changed
|
||
export default class MirrorCovers extends Component { | ||
static propTypes = { | ||
/** Mirror Covers view width */ |
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.
Good job 👍️
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.
If all Sebastian comments were resolved, I approve this PR. Excellent job @fdelcampo !!
4d48d46
to
9be36b0
Compare
…Summary component
…er with degree in variable. Pending evaluation of cases
…tate with different of angles for the difference states
9be36b0
to
8c80fb8
Compare
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.
Everything looks good and ready for merge! Great job @fdelcampo
This PR contains TMA Implementation.