Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Keypad should use nested options for its NumberAccumulator #541

Closed
zepumph opened this issue Oct 11, 2019 · 6 comments
Closed

Keypad should use nested options for its NumberAccumulator #541

zepumph opened this issue Oct 11, 2019 · 6 comments

Comments

@zepumph
Copy link
Member

zepumph commented Oct 11, 2019

Discovered while working on #540. Tagging @Denz1994 to see what he thinks.

@zepumph
Copy link
Member Author

zepumph commented Oct 11, 2019

Right now [in Keypad.js] I have to omit the parent tandem before passing in the child one:

    // @private {function}
    this.keyAccumulator = options.accumulator ? options.accumulator : new NumberAccumulator( merge( {
      tandem: options.tandem.createTandem( 'numberAccumulator' )
    }, _.omit( options, 'tandem' ) ) );

@pixelzoom
Copy link
Contributor

pixelzoom commented Feb 23, 2021

I don't see any opportunities for using nested options in NumberAccumulator.

@zepumph Do you mean that KeyPad should use nested options for its NumberAccumulator subcomponent?

EDIT: I'm going with "yes".

@pixelzoom pixelzoom changed the title Keypad's NumberAccumulator should use nested options Keypad should use nested options for its NumberAccumulator Feb 23, 2021
@pixelzoom pixelzoom assigned pixelzoom and unassigned zepumph Feb 23, 2021
@pixelzoom
Copy link
Contributor

I'm going to work on this issue, self assigning.

@pixelzoom
Copy link
Contributor

pixelzoom commented Feb 23, 2021

There are 5 occurrences of import Keypad from (in 4 repos) to be inspected for use of NumberAccumulator options:

  • area-model-common
  • collision-lab
  • fourier-making-waves
  • projectile-motion
  • scenery-phet demo

The complete set of NumberAccumulator options (including AbstractKeyAccumulator superclass) is:

  • additionalValidator
  • maxDigits
  • maxDigitsRightOfMantissa
  • tandem

pixelzoom added a commit to phetsims/fourier-making-waves that referenced this issue Feb 23, 2021
pixelzoom added a commit to phetsims/projectile-motion that referenced this issue Feb 23, 2021
pixelzoom added a commit to phetsims/collision-lab that referenced this issue Feb 23, 2021
@pixelzoom
Copy link
Contributor

Options accumulatorOptions was added to Keypad in the above commit, and adjustments were made to sims. area-model-common required no adjustments because it provides options.accumulator to Keypad, therefore making options.accumulatorOptions irrelevant.

In additon to testing all sims that use Keypad, I inspected the Studio tree in projectile-motion (screenshot below) and it looks correct.

@zepumph would you mind doing a quick review, since you created this issue?

screenshot_881

@zepumph
Copy link
Member Author

zepumph commented Feb 26, 2021

Looks really nice. Thanks

@zepumph zepumph closed this as completed Feb 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants