Skip to content

Commit

Permalink
Various review notes and code cleanup, see #29.
Browse files Browse the repository at this point in the history
  • Loading branch information
jbphet committed Dec 3, 2014
1 parent 8842520 commit ca01263
Show file tree
Hide file tree
Showing 3 changed files with 22 additions and 16 deletions.
23 changes: 14 additions & 9 deletions js/neuron/chart/view/MembranePotentialChart.js
Original file line number Diff line number Diff line change
Expand Up @@ -74,12 +74,12 @@ define( function( require ) {
function MembranePotentialChart( chartDimension, neuronClockModelAdapter ) {

var thisChart = this;
Node.call( thisChart, {} );
Node.call( thisChart );
thisChart.chartDimension = chartDimension;
thisChart.clock = neuronClockModelAdapter;
thisChart.neuronModel = neuronClockModelAdapter.model;

thisChart.updateCountdownTimer = 0; // Init to zero to an update occurs right away.
thisChart.updateCountdownTimer = 0; // Init to zero so that an update occurs right away.
thisChart.timeIndexOfFirstDataPt = 0;
thisChart.pausedWhenDragStarted = false;
thisChart.dataSeries = new XYDataSeries( {color: 'red'} );
Expand All @@ -100,7 +100,7 @@ define( function( require ) {
var domainMap = new dot.LinearFunction( 0, numVerticalGridLines, this.domain[0], this.domain[1] );

//To create Vertical Labels
// Example:- for the value of 3 it returns a value of -50 and for 5 it returns 0 (because range is between -100 t0 100)
// Example:- for the value of 3 it returns a value of -50 and for 5 it returns 0 (because range is -100 to 100)
var rangeMap = new dot.LinearFunction( 0, numHorizontalGridLines, this.range[1], this.range[0] );


Expand Down Expand Up @@ -138,8 +138,7 @@ define( function( require ) {
thisChart.neuronModel.stimulusPulseInitiatedProperty.link( function( stimulusPulseInitiated ) {
if ( stimulusPulseInitiated ) {
if ( !thisChart.neuronModel.isPotentialChartVisible() ) {
// If the chart is not visible, we clear any previous
// recording.
// If the chart is not visible, we clear any previous recording.
thisChart.clearChart();
}
// Start recording, if it isn't already happening.
Expand All @@ -151,9 +150,11 @@ define( function( require ) {
var chartTitleNode = new Text( chartTitleString, {font: new PhetFont( {size: 16, weight: 'bold'} )} );
var clearChartButton = new TextPushButton( chartClearString, {
font: new PhetFont( {size: 12} ),
baseColor: 'silver',
//REVIEW - FYI: I deleted the color so that the default would be used. The gray looked disabled.
//REVIEW - Why is the flat strategy used? I tried removing this to use the default strategy, and thought it looked good. Any reason?
buttonAppearanceStrategy: RectangularButtonView.flatAppearanceStrategy
} );
//REVIEW - the listener can be added as an option during the construction step above, for slightly cleaner code.
clearChartButton.addListener( function() {thisChart.clearChart();} );

var closeIconRadius = 4;
Expand All @@ -167,6 +168,7 @@ define( function( require ) {
var closeButton = new RectangularPushButton( {
baseColor: 'red',
content: xIcon,
//REVIEW - Why is the flat strategy used? I tried removing this to use the default strategy, and thought it looked good. Any reason?
buttonAppearanceStrategy: RectangularButtonView.flatAppearanceStrategy,
// click to toggle
listener: function() {
Expand Down Expand Up @@ -249,8 +251,8 @@ define( function( require ) {
);


thisChart.neuronModel.potentialChartVisibleProperty.link( function( chartVisibile ) {
thisChart.visible = chartVisibile;
thisChart.neuronModel.potentialChartVisibleProperty.link( function( chartVisible ) {
thisChart.visible = chartVisible;
} );


Expand All @@ -265,6 +267,7 @@ define( function( require ) {
* @param voltage - Voltage in volts.
* @param update - Controls if graph should be refreshed on the screen.
*/
//REVIEW - the update param appears to be unused, can it be removed?
addDataPoint: function( time, voltage, update ) {

if ( this.dataSeries.length === 0 ) {
Expand All @@ -289,7 +292,7 @@ define( function( require ) {
this.chartIsFull = true;
}
else {
console.log( "MembrancePotential Chart Warning: Attempt to add data to full chart, ignoring." );
console.log( "MembranePotential Chart Warning: Attempt to add data to full chart, ignoring." );
}
},

Expand All @@ -305,12 +308,14 @@ define( function( require ) {
}
return timeOfLastDataPoint;
},

/**
* Update the chart based on the current time and the model that is being
* monitored.
*
* @param dt
*/
//REVIEW - Parameter docs don't match.
step: function( simulationTimeChange ) {

if ( this.neuronModel.isRecord() ) {
Expand Down
14 changes: 7 additions & 7 deletions js/neuron/controlpanel/AxonCrossSectionControlPanel.js
Original file line number Diff line number Diff line change
Expand Up @@ -60,17 +60,17 @@ define( function( require ) {
expandTouchArea( showPotentialChartCheckBox );


var crossectionControlContents = [];
crossectionControlContents.push( new Text( showLegendString, {
var crossSectionControlContents = [];
crossSectionControlContents.push( new Text( showLegendString, {
font: new PhetFont( { size: 16, weight: 'bold' } )} ) );
crossectionControlContents.push( allIonsSimulatedCheckBox );
crossectionControlContents.push( showChargesCheckBox );
crossectionControlContents.push( showConcentrationsCheckBox );
crossectionControlContents.push( showPotentialChartCheckBox );
crossSectionControlContents.push( allIonsSimulatedCheckBox );
crossSectionControlContents.push( showChargesCheckBox );
crossSectionControlContents.push( showConcentrationsCheckBox );
crossSectionControlContents.push( showPotentialChartCheckBox );

// vertical panel
Panel.call( this, new VBox( {
children: crossectionControlContents,
children: crossSectionControlContents,
align: 'left',
spacing: 5
} ), {
Expand Down
1 change: 1 addition & 0 deletions js/neuron/model/NeuronClockModelAdapter.js
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,7 @@ define( function( require ) {
/**
* Registers a callback that will be notified when the clock is reset
*/
//REVIEW - this looks like a typo - should be "Reset" instead of "Rest", correct?
registerRestCallback: function( callback ) {
this.resetCallBacks.push( callback );
},
Expand Down

0 comments on commit ca01263

Please sign in to comment.