phetsims / beers-law-lab Goto Github PK

When the shaker is completely emptied and a new solute is selected from the drop down menu, it took about 6 seconds on the iPad3 to respond, which made it look like the drop down menu was broken.

Steps to reproduce:

1. Load 1st tab (Concentration)
2. Select "Solution" (dropper)
3. Place the Concentration meter's sensor directly below the dropper (centered and above the liquid)
4. Press the dropper's button (liquid comes out, concentration meter properly reads the maximum concentration for the solute).
5. Release the dropper's button.

Expected: Concentration meter goes back to zero (if it's still above the liquid level)
Actual: Concentration meter stays at the maximum concentration for the solute.

This was reported by @jonathanolson in #16, moved to this new ticket to track separately.

In beers-law-lab-config.js I saw this shim:

  shim: {
    jquery: { exports: "$" }
  },

But I think it is unused and should be removed.

If you load the Concentration tab on a tablet, and drag both the evaporation slider and top faucet (so that it is evaporating faster than water is being added), once it hits zero fluid evaporation will stop and it will start filling.

When you move the finger that was on the evaporation slider (still pressed down), it will switch to that rate of evaporation and start evaporating again.

Fluid is currently defined as:

  // imports
  var Property = require( "AXON/Property" );

  /**
   * @param {Color} color
   * @constructor
   */
  function Fluid( color ) {
    this.color = new Property( color );
  }

  Fluid.prototype = {
    reset: function() {
      this.color.reset();
    }
  };

  return Fluid;

If it were rewritten to use PropertySet, it would be defined as:

  // imports
  var PropertySet = require( "AXON/PropertySet" );
  var inherit = require( 'PHET_CORE/inherit' );

  /**
   * @param {Color} color
   * @constructor
   */
  function Fluid( color ) {
    PropertySet.call( this, {color: color} );
  }

  return inherit( PropertySet, Fluid );

The latter makes it possible to get/set the value directly, using

thisSolution.color = ConcentrationSolution.createColor( thisSolution.solvent, thisSolution.solute.get(), thisSolution.concentration.get() );

instead of

thisSolution.color.set( ConcentrationSolution.createColor( thisSolution.solvent, thisSolution.solute.get(), thisSolution.concentration.get() ) );

Property links would also need to be changed, if you convert to PropertySet. Not a big deal since there is just one property in this case, but just thought I would bring it up discussion because (a) it relates to uniformity between sims (b) if you add other properties to Fluid later, it will manage them well (automatically remember to reset them, etc) (c) the constructor is a bit shorter and (d) es5 get/set can be more convenient that Property.get() and Property.set()

It is not clear from the code or comments why VisibleColor.colorTable is lazily created. I recommend to create it eagerly or document why it is necessary to create lazily. Also, I tested creating the table eagerly and did not see any perceptible performance difference on Win8/Chrome on iPad3/Safari.

A close cousin to #22 for multi-touch (on a tablet)

1. Switch to the dropper
2. Move the concentration meter directly under the dropper
3. Start the dropper's flow with finger A (hold down). Concentration goes up to match the dropper's concentration.
4. Switch to the shaker with finger B (keep holding finger A)
   The concentration meter should still read the high value, while being above the beaker's fluid level in shaker mode. In addition, it keeps filling.

We should figure out a good way to stop the shaker/dropper action when the mode switches.

1. Open the first tab, and drag out the concentration meter with 1 finger (keep it held down)
2. Place three other fingers down, and drag all four to the right/left. This will trigger swapping between applications on iOS, and should issue an immediate touchcancel event.

Expected: Concentration meter drag stops, and stays where it is.
Actual: Concentration meter sensor moves away from the wire, and the next drag on it is offset a significant amount.

Thank you to RSC text reads "Conversation of this simulation..." when it should read "Conversion of this simulation...

See red horizontal lines at bottom of cuvette in screenshot below. Similar lines with other solutions, with the lines matching the color of the solution.

iPad2, iOS 6.1.3, Safari 6.0. Happens 100% of the time. Have not seen this on other platforms.

ConcentrationControl uses this bit of ugly code, which assumes that the value is 3 digits, and the units are 3 characters (XXX).

var valueDisplay = new Text( '400 XXX', { font: FONT } ); //TODO too many assumptions here, and doesn't work for i18n

I recall we discussed single vs double quotation marks a while ago. In the interest of consistency (making it easy to search and find things) I am leaning toward single quotation marks, and double quotation marks for nested/quoted strings. Not a big deal but thought I would bring it up for discussion as part of this code review.

... when we figure out how to support common-code localization.

EvaporationSlider seems like a reasonable starting point for a SUN slider component, see phetsims/sun#9. Perhaps we should migrate it.

When shaking in lots of solute, it shows the text "saturated!". If you try to shake more solute then, no more solute comes out. However, if it says "saturated" you can continue to shake more and more solute from the shaker if you were already shaking it. Should we change this behavior to make it so the shaker runs out of solute when saturated, while you are shaking it?

Tested on iPad3, iOS7, Mobile Safari 7.

Firefox 22.0

Two things noticed:

(1) There's a vertical while line between the flange and the shaft, perhaps a tiny bit of horizontal space?

(2) After dragging the faucet knob, it sometimes shifts up or down a pixel or two when you release it.

ArrowButton declares setEnabled on an instance-by-instance basis instead of once in the prototype. If there is a good reason for this, it should be documented. Otherwise, I would recommend to move it to the prototype.

http://www.colorado.edu/physics/phet/dev/html/beers-law-lab/1.0.0-rc2/beers-law-lab.html
Nexus 7
Android 4.3
Firefox 23.0

... when we figure out a strategy for common-code images.

The sim freezes on Firefox 21.0 on Nexus 7 Android 4.2.2

Performance however rocks with Chrome.

If and when phetsims/scenery#66 is addressed, remove the workaround that keeps track of the drag target. Specifically, delete this like and any lines that depend on it:

var target = null; // workaround for scenery#66 (currentTarget is null in drag function)

There also has been talk of deprecating SimpleDragHandler, but that becomes less likely as it proliferates.

Using link http://phet.colorado.edu/sims/html/beers-law-lab/latest/beers-law-lab_en.html, FF 22.0, Nexus 7, Android 4.2.2, the sim no longer freezes, but there is odd drag handling. For instance, when pressing the dropper the button gets stuck, and swiping in other places can move the dropper. In general, the sim is difficult to use, but performance is OK (shaker particles and such). The sim look great on Chrome however.

HorizontalTiledNode currently adds multiple copies of an image to its center. For improved performance and a less complex implementation, I would recommend to reimplement using scenery.Pattern.

version.js contains this declaration:

define( function( require ) {
  "use strict";
  return function() { return "0.0.0-13"; };
} );

I recommend (in this case and others) to omit unused function parameters (in this case require), it just seems like a faint code smell.

ArrowButton declares

  // direction that the arrow points
  ArrowButton.Direction = {
    'LEFT': 0,
    'RIGHT': 1
  };

And the client side usage is:

new ArrowButton( ArrowButton.Direction.LEFT, ...

I fail to see the advantage of this additional code. IMO the javascripty way to do this is:

new ArrowButton( 'left', ...

If after discussion we decide to keep the enum pattern, then the quotes around 'LEFT' and 'RIGHT' should be removed.

Firefox 22.0. Resize the browser window while on the "Concentration" tab, and you'll see the shaker jumping between its correct position and the upper-left corner of the window. When done resizing the shaker is in its correct position.

Mac OS 10.8.4, Firefox 22.0, BLL 1.0.0 ('svg' renderer).

Value displayed in the AT Detector shifts up and down as it changes.

In 3 consecutive runs on Win8/Firefox24, No solute particles appeared on the right side of the beaker.

They go all the way to the left side of the beaker so it is a bit asymmetric.

When swiping across to drag the shaker for the first time, the yellow arrows appear and disappear as the pointer moves on/off from the beaker (happens multiple times). If you just tap the beaker to drag it (instead of swiping across) the arrows flicker on then off.

Troubleshooting information
Name: Concentration
URL: http://www.colorado.edu/physics/phet/dev/html/concentration/1.0.0-rc.1/concentration_en.html
Version: 1.0.0-rc.1
Features missing: fullscreen
Flags: pixelRatioScaling
User Agent: Mozilla/5.0 (iPad; CPU OS 7_0 like Mac OS X) AppleWebKit/537.51.1 (KHTML, like Gecko) Version/7.0 Mobile/11A465 Safari/9537.53
Language: en-us
Window: 1024x672
Pixel Ratio: 2/1

beers-law-lab.html has these lines, which should be removed:

<!-- CSS -->
<link rel="stylesheet" href="../phetcommon/css/phetcommon.css"/>

See also phetsims/phetcommon#9

ArrowButton.js cannot be scaled, positioned, rotated, opacity changed, etc by passing in an options object, even thought it supports an options object (for other options). If this is truly destined for common code, then it should probably support those options.

Navbar was absent when she was using the chrome browser. Not sure if she's got the latest software. I'll try to recreate this on another iPad when I get ahold of one.

Win7 Home Premium, 64-bit, 6.2 (Build 7600)
IE 9.0.8112.16421 (update 9.0.12)
Beer's Law Lab 1.0.0 (renderer:svg)

See attached screenshot. Text looks awful.

Testing: http://phet.colorado.edu/sims/html/beers-law-lab/latest/beers-law-lab_en.html
Two different versions of this bug exist depending on the OS/Chrome version:

V1 - Observed in Mac OSX 10.8.4, Chrome 28.0.1500.71
Spacing between characters in Remove Solute label increases after mouse-over of button. Changes back to original kerning after use of button.

ORIGINAL SPACING

AFTER MOUSEOVER

V2 - Observed in Chromebook, Chrome v.28.0.1500.68
Spacing between characters in Remove Solute text decreases after use of the button. Does not change back. See attached screenshots.

BEFORE

AFTER

It gets darker while it is changing size. Once stable, it switches back to the correct color.

See phetsims/scenery#125 for upstream Scenery info

On the windows 8 laptop with IE version 10.0.9200 testing 0.0.0-17 there is a misalignment of the variable wavelength control. See screenshot:

Reported by Trish via email:

Trish computer: Windows 7 Professional Dell Vostro 3300 laptop, i5 CPU M460 @2.53 Ghz, 32 bit operating system, NVIDIA GeForce 310 M,

IE 9.1.06 everything looks fine

Chrome Version 27.0.1453.110 m – 1. label on concentration nM – blend together 2. Bottom bar cuts off when resized 3. blurry text 4. Chemical names cut off see in particular cobalt chloride

Firefox version 21.0 – 1. chem names font inconsistent tab 1 to tab 2 2. Text on grey control panels is not very distinct because fine lined (the white on green might look poor on the image capture, but it is more clear on my screen)

Chrome:

Firefox:

Chrome:

Firefox:

Firefox:

IE:

Chrome:

Chrome:

IE:

Chrome:

IE:

Issue to track general Scenery performance issues on the sim.

Main one seems to be the 5 FPS I'm getting on the iPad 3 when dragging the shaker on the 1st tab (almost assuredly due to the particles, I'll look into performance).

The 2nd tab's drop-down menu also seems to be somewhat slow opening and closing.

See the screenshot below:

On the 2nd tab, the "Reset All" button is probably uncomfortably close to the side, but it's not as much of an issue.

When creating new ArrowButton( ArrowButton.Direction.RIGHT,... it actually points to the left and moves the slider to the left (and likewise LEFT points right). Am I confused or should this be fixed?

On the Surface with version 0.0.0-17 and all three browsers (Chrome, IE, Firefox) there is some weirdness associated with touch and hold... for instance when you use the tweakers in "fire on hold" mode. In chrome the tweaker stays active when you release it, in IE you can end up bringing up a right click menu, and so on...

Such a quirky device...

Firefox 22.0.

Drag the shaker and its text label gets bolder. Release it and it returns to normal. Also gets bolder while resize the browser window.

Beers law lab Toggle Button looks like it wants to fill the same role as sun.ToggleButton. We should try to coalesce them. Things to keep from sun.ToggleButton? It supports any node as content, not just images. Things to keep from BLL.ToggleButton? Enabled/disabled flag.

BLLSymbols is currently defined as:

  function BLLSymbols() {}

  BLLSymbols.COBALT_II_NITRATE = toSubscript( "Co(NO3)2" );
  ...
  BLLSymbols.WATER = toSubscript( "H2O" );
  return BLLSymbols;

But it is a bit confusing because it is always accessed statically and never as a function. I would recommend to rewrite as:

return {COBALT_II_NITRATE:toSubscript( "Co(NO3)2" ),...};

Title of the sim on the homescreen (in both BLL and BAA) appears in the serif font (shown below) rather than the sans serif shown in other browsers or in Chrome on any other OS other than the Chromebook.

Of all the platforms I tested, IE9 animation looked the worst. It's usable, but not pretty.

Win7 Home Premium, 64-bit, 6.2 (Build 7600)
IE 9.0.8112.16421 (update 9.0.12)