phetsims / balloons-and-static-electricity Goto Github PK

Currently bounds is declared as:

this.bounds = [this.sweater.center.x, 0, width - this.wallWidth, height];

Which leads to code like:

  if ( newLocation.y + balloonModel.height > model.bounds[3] ) {
    isStopped = true;
    newLocation.y = model.bounds[3] - balloonModel.height;
  }
  if ( newLocation.x < model.bounds[0] ) {
    isStopped = true;
    newLocation.x = model.bounds[0];
  }
  if ( newLocation.y < model.bounds[1] ) {
    isStopped = true;
    newLocation.y = model.bounds[1];
  }

I would recommend switching to an object literal or DOT/Bounds2 so code would read like bounds.maxY instead of bound[3] to make it easier to understand.

The sweater should not overlap the control buttons, as in the mockup.

Instead of bootstrap, make the “1” vs “2” balloons buttons using scenery paths & images, as shown in the mockup, so that the selected button looks “pressed in” and the unselected button looks “popped up” with a drop shadow.

The "show all charges" and adjacent radio buttons should have ratio button circle icons, as shown in the mock up.

In BalloonsAndStaticElectricityModel.js line 48 do you think we could use

var curTime = Date.now();

instead of

var curTime = new Date().getTime();

?

"1 vs 2" balloon radio buttons should be swapped left to right, see the screen shot.

One of several layout issues in Firefox.

The background is currently white. It should be changed to the same shade of blue as in the mock up. RGB = (157,220,248) #9ddcf8

Please reduce the lineWidth of the “1 vs 2” balloons radio buttons border stroke to match the mockup more closely. It looks like it may be 3 now but should be 2 or so like in the mockup. Here is a comparison:

When the sim starts, and just one balloon is showing (see #10 ) the string should be vertical, indicating that the balloon is pulling straight up instead of to the side.

In html5 version new string appeared. Need translate them to all languages.
List below (from balloons-and-static-electricity-strings.js):

        "BalloonApplet.removeWall": "Remove<br/> Wall",
        "BalloonApplet.addWall": "Add<br/> Wall",
        "BalloonApplet.resetBalloon": "resetBalloon",
        "BalloonApplet.resetBalloons": "resetBalloons"

The "Add Wall" button text should be two lines, as shown in the mock up. The "Add Wall" and "Remove Wall" buttons should be the same size and location.

When moving a charged balloon toward the wall, the wall charges oscillate quite a long time (several seconds) and it is distracting. Can damping be added to its oscillation so it will won't oscillate too long?

Balloons should be smaller in size to match mock up

I'm going to take a quick look through the current balloons-and-static-electricity code and make review comments and minor fixes in this ticket.

Balloon should be exactly 20% larger in size to match mockup

Please move the balloon string to the left so it does not overlap the "1 vs 2" balloon radio button, and has some space to the left as in the mockup.

When both balloons are charged, one balloon can chase the other off the bottom of the screen. The bottom of the screen should have a boundary (like the top of the screen) so the balloons cannot escape.

The sim should start with a single balloon (they yellow one) instead of both.

The "1 vs 2" balloon radio button should be centered over the "reset balloon" button.

On Win8/Chrome, F11 full screen caused the background to turn from blue to white once. Is this problem reproducible?

Right now the simulation depends on submodules of scenery, but I think it should be changed so it is using peers for everything, including scenery's dependencies. This will require developers to have all of the dependencies listed in the *config.js checked out in the same directory.

I believe it will be more maintainable and reusable if the "1 vs 2" balloon pushbutton radiobuttons were implemented in Scenery. This was also requested in the original specification https://github.com/phetsims/balloons-and-static-electricity/tree/master/doc : the 1 vs 2 balloon buttons should be created using Scenery

The buttons on the right (reset all and remove wall) cannot be pressed because the radio buttons div is too wide. Can it be manually sized so it won't overlap them?

I didn't understand this comment in the aforementioned file //if we write 0 instead of "0", then parameter becomes object, not number, can you clarify or remove the comment if it is no longer applicable?

The reset all button gets overlapped when the text on the "reset balloon" button changes to "reset balloons". Noah will see if he can come up with a sensible icon or pictograph to replace the text (which will also help with i18n layout issues).

Many of the methods in BalloonModel.js would benefit from more documentation (code comments) to explain what is happening and why.

If it is easy, please add black stroke outline to Reset Balloon and Remove Wall bootstrap buttons.

The "reset balloon" button only resets the charges to their initial state. This is different from "reset all", which resets the entire sim back to the initial state.

It takes more swipes to remove more charges from the sweater than in the original Java version. Please make it easier for the balloon to pick up charges when dragged across the sweater, to match the behavior in the original Java version.

Charges should be exactly 30% larger in size to match mockup

BalloonModel.getSweaterForce currently takes 2 arguments, the sweaterModel and the balloonModel. Shouldn't it just take a sweaterModel argument and use the 'this' as the balloonModel, so you can call balloon.getSweaterForce(sweater)?

If you move the balloon to the edge of the window, then hold down the mouse button and continue to move the cursor, then bring the cursor back to the window the balloon moves before the mouse cursor gets to it. This makes the balloon become detached from the mouse cursor.

The behavior should be as in the original Java version, where the balloon only moves when the mouse cursor is over it.

Our artist gave us a new sweater, but it is not rendering the texture properly on iOS. @jonathanolson helped fix rendering problems on IE but they did not solve it on iOS. We should either (a) fix the SVG for iOS or (b) use a raster.

The bootstrap radio buttons under the sweater look fuzzy on chrome, and get cropped in awkward ways depending on the zoom level of the browser. At our 6/12/2013 review, Noah & I decided to rewrite them using sun components.

In SweaterModel.js, the require statement says var PointCharge = require( 'model/PointChargeModel' );. In this case and others, the var name should match the required filename to facilitate searches and code navigation.

The balloon moves too quickly when it is charged and released. Reduce the acceleration of the balloon so that it moves more slowly, to match the behavior in the original Java version.

This method in BalloonsAndStaticElectricityModel.getBalloonRestrictions would benefit from some code comments. Also, it is a bit odd that it is a get function, but what it actually does is attaches a value to one of the arguments and returns the argument. May be confusing.

        getBalloonRestrictions: function( position, objWidth, objHeight ) {
          var rightBound = this.width;
          var isOutBounds = false;
          if ( this.wall.isVisible ) {
            rightBound -= this.wallWidth;
          }

          if ( position.x + objWidth > rightBound ) {
            position.x = rightBound - objWidth;
            isOutBounds = true;
          }

          if ( position.y < 0 ) {
            position.y = 0;
            isOutBounds = true;
          }

          if ( position.x < 0 ) {
            position.x = 0;
            isOutBounds = true;
          }
          else if ( position.y + objHeight > this.height ) {
            position.y = this.height - objHeight;
            isOutBounds = true;
          }
          position.isOutBounds = isOutBounds;
          return position;
        }

The initial position of the yellow balloon should be lower on the screen, as shown in the mockup. Approximately the middle of the sweater vertically.

Can you please add code comments to describe the positions in SweaterModel? Where did these values come from, what do they mean, and what might cause them to change in the future?

In BalloonModel.js, what is the xVelocityArray and why is it needed? Please add code comments for it.

Text for all buttons should be black instead of gray, see screenshot.

The balloon strings touch at the top of the title bar. Instead, the point of intersection should be moved down below (behind) the title bar where it cannot be seen, as in the mockup.

The "Remove Wall" and "Add Wall" button colors should match the mock up (orange gradient for Remove Wall and greed gradient for Add Wall)

HTML elements not scales correctly in Firefox (they are added in ControlPanel.js).
In IE and Chrome its ok.

Text for the "show all charges" and adjacent buttons should be left aligned as shown in the mock up.

In BalloonModel.js please document the array of positions and positions of start changes (what do the values mean and where did they come from, what possible reasons might come up for regenerating the array in the future).