Test TimerNode to make sure it still works properly in Capacitor Lab: Basics

[samreid]

From https://github.com/phetsims/scenery-phet/issues/372#issuecomment-421099482 and also related to #234. The TimerNode has recently been updated to support units, and will require testing to make sure it still works properly in Capacitor Lab: Basics.

Also assigning @Denz1994 since I saw he had been working in #234

[samreid]

I tested the TimerNode in master Capacitor Lab: Basics and did not see any issues. I tried to compare this to the previous published version, but the previous published version does not have a TimerNode. Assigning to @Denz1994 since he added the DraggableTimerNode in Capacitor Lab: Basics.

[Denz1994]

After testing with DraggableTimerNode, things work well in the sim. The units option seems flexible enough to handle long strings and as noted in the documentation does change the format of the TimerReadout.

[Denz1994]

A few nuances I noticed during testing. TimerReadoutNode.maxValue has some weird outcomes when negative values are entered. For example:

    TimerNode.call( this, secondsProperty, timerRunningProperty, {
      maxValue: -20,
      unitsNode: new Node( { children: [ new Text( 'test' ) ] } ),
      tandem: tandem.createTandem( 'timer' )
    } );

The above code produces this:

The buttons become unresponsive, but don't trigger any errors. Perhaps an assertion for rejecting negative numbers may be helpful.

[Denz1994]

Additionally, extremely large values are generally handled well, but there are some more edge cases that I've noticed.

Any entry in maxValue after 17 digits the value gets rounded in the readout and when reset the timer doesn't change size when the value is zero.

This may be out of the scope of this issue, but it does look a bit wierd that the timerNode remains that large despite having a value with significantly less digits. I've noticed that the timer readout uses scientific notation once the value has 22 digits. This seems pretty high for most uses cases and it may be better to start using scientific nototation after 7 digits? Thoughts @samreid.

[Denz1994]

The elongated TimerNode is shown above only appears if there is a unique maxValue and unitsNode option passed. If only one of the options is passed at initialization, then the TimerNode doesn't widen.

The behavior isn't consistent and either the timer should widen or not, but currently, it does both depending on the options passed.

[samreid]

This may be out of the scope of this issue, but it does look a bit wierd that the timerNode remains that large despite having a value with significantly less digits.

I believe this design is so that the TimerNode can accommodate the largest string without any text scaling and without resizing the text box.

I've noticed that the timer readout uses scientific notation once the value has 22 digits. This seems pretty high for most uses cases and it may be better to start using scientific nototation after 7 digits?

It seems unlikely that we would want to automatically transition from non-exponential notation to exponential notation in a timer. Should we throw an exception if maxValue is greater than or equal to 1E+21 (which is the JavaScript default according to http://2ality.com/2012/03/displaying-numbers.html)?

The elongated TimerNode is shown above only appears if there is a unique maxValue and unitsNode option passed. If only one of the options is passed at initialization, then the TimerNode doesn't widen. The behavior isn't consistent and either the timer should widen or not, but currently, it does both depending on the options passed.

The timer uses the maxValue and the bounds of the unitsNode to determine the size of the text container, see https://github.com/phetsims/scenery-phet/blob/5ccc58d1ed303b76f0fd557473e78300bbfd8ddb/js/TimerReadoutNode.js#L90-L91. Perhaps I'm not understanding the inconsistency, can you please elaborate and provide an example?

[Denz1994]

If the intent of design is to avoid box resizing then, that paradigm should be followed.

Should we throw an exception if maxValue is greater than or equal to 1E+21?

An assertion sounds like suitable addition in this case. It wasn't clear to me why at 22 digits scientific notation is used, but the link you provided explains it well ("Displaying Decimal Numbers" section).

Perhaps I'm not understanding the inconsistency?

The documentation you pointed out is clear that the maxValue is responsible for the panel size. This is less of an inconsitency and more of an oversight in reviewing documentation on my end. Thanks for the clarification.

Re-assigning to @samreid.

EDIT by @samreid to separate responses.

[samreid]

I added an assertion, back to you for re-review. Close if all is well, thanks!

[Denz1994]

Looks great. Thanks @samreid.