Attitude bar view#173
Conversation
| }, | ||
|
|
||
| initialize: function () { | ||
| this.degree_or_rad = false; // false stands for degree |
There was a problem hiding this comment.
Probably makes more sense to name this to this.is_rad
| initialize: function () { | ||
| this.degree_or_rad = false; // false stands for degree | ||
| this.aircraft_position_callback = this.aircraftPositionCallback.bind(this); | ||
| TelemetryData.addListener('aircraft_position', this.aircraft_position_callback); |
There was a problem hiding this comment.
Telemetry listeners should occur within the onRender method
| var rate = this.ui.pitch_input.val(); | ||
| if(rate==null) { | ||
| rate = 0; | ||
| }else{ |
There was a problem hiding this comment.
Add a space after and before the curly brackets in the conditional statements
| </span> | ||
| </div> | ||
| <!-- <p>Yaw Setpoint: <span class="yaw-setpoint-value">--</span> °</p> --> | ||
| <div class="attitudeBarView pitch component"> |
There was a problem hiding this comment.
You're not using classes properly. If you've got nested components they don't need to be the same class
| } | ||
|
|
||
| .attitudeBarView.roll.block{ | ||
| //margin-top: 10%; |
There was a problem hiding this comment.
This is invalid css (not how you make comments in css, not should you keep commented code in pull requests.)
| @@ -0,0 +1,16 @@ | |||
| var AttitudeBarView = require('../../app/views/AttitudeBarView')(Marionette, Backbone); | |||
There was a problem hiding this comment.
File name wrong for this file. This one is a window, the other you got correct which are views. Ie. Should be attitude-bar-window.js
…e approrpiate. Delete unecessary models require. Changed HTML layout and css.
sergei1152
left a comment
sergei1152
left a comment
There was a problem hiding this comment.
- Can you make the input box and button styling consistent with the main window
- The yaw angle text (below the yaw rate) isnt used or needed)
- The default size of the window is pretty big considering what it has to display. It also looks bad (the text overflows) with smaller sizes). If you can make it look nice on a 500wx600h size, that'd be ideal
- Having the bar graph filled on the bottom doesnt make too much sense, considering the reference is 0 rad/s, and not negative. I'll show you what i mean on the whiteboard
|
|
||
| sendPitchRate: function(e) { | ||
| e.preventDefault(); | ||
| var rate = this.ui.pitch_input.val(); |
There was a problem hiding this comment.
theres no input checking on this at all. CHeck out the Validator module for an appropriate check (ie. the isNumeric check should probably work for this)
There was a problem hiding this comment.
because currently you can enter a blank space to the input box and it would be sent
| } else { | ||
| rate = rate %360; | ||
| } | ||
| console.log(Commands.sendYawRate(rate)); |
There was a problem hiding this comment.
console logs should only be used for development
| pitch_rate_deg: '.pitch.value-deg', | ||
| pitch_front: '.pitch.block.front', | ||
| pitch_input: '.input.pitch', | ||
| yaw: '.yaw-value', |
There was a problem hiding this comment.
this is not used or updated. THe yaw value is not needed in this view, just the rates, so you can delete this as well as the corresponding html element.
2 participants
A window for displaying attitude change rates in bar-shaped GUI.