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

Compress Key layouts #553

Closed
samreid opened this issue Nov 7, 2019 · 5 comments
Closed

Compress Key layouts #553

samreid opened this issue Nov 7, 2019 · 5 comments

Comments

@samreid
Copy link
Member

samreid commented Nov 7, 2019

While working on #540 I came across the key layouts, which look like this:

    PositiveIntegerLayout: [
      [ new Key( '7', KeyID.SEVEN ), new Key( '8', KeyID.EIGHT ), new Key( '9', KeyID.NINE ) ],
      [ new Key( '4', KeyID.FOUR ), new Key( '5', KeyID.FIVE ), new Key( '6', KeyID.SIX ) ],
      [ new Key( '1', KeyID.ONE ), new Key( '2', KeyID.TWO ), new Key( '3', KeyID.THREE ) ],
      [ new Key( '0', KeyID.ZERO, { horizontalSpan: 2 } ), new Key( ( new BackspaceIcon( { scale: 1.5 } ) ), KeyID.BACKSPACE ) ]
    ],

    PositiveAndNegativeIntegerLayout: [
      [ new Key( '7', KeyID.SEVEN ), new Key( '8', KeyID.EIGHT ), new Key( '9', KeyID.NINE ) ],
      [ new Key( '4', KeyID.FOUR ), new Key( '5', KeyID.FIVE ), new Key( '6', KeyID.SIX ) ],
      [ new Key( '1', KeyID.ONE ), new Key( '2', KeyID.TWO ), new Key( '3', KeyID.THREE ) ],
      [ new Key( ( new BackspaceIcon( { scale: 1.5 } ) ), KeyID.BACKSPACE ), new Key( '0', KeyID.ZERO ), new Key( PLUS_CHAR + '/' + MINUS_CHAR, KeyID.PLUS_MINUS ) ]
    ],

Since the Key seems to be a data type, it may be possible to factor out the keys like so:

  const _7 = new Key( '7', KeyID.SEVEN );
  const _8 = new Key( '8', KeyID.EIGHT );
  const _9 = new Key( '9', KeyID.NINE );
  const _4 = new Key( '4', KeyID.FOUR );
  const _5 = new Key( '5', KeyID.FIVE );
  const _6 = new Key( '6', KeyID.SIX );

  const _1 = new Key( '1', KeyID.ONE );
  const _2 = new Key( '2', KeyID.TWO );
  const _3 = new Key( '3', KeyID.THREE );

  const backspaceKey = new Key( ( new BackspaceIcon( { scale: 1.5 } ) ), KeyID.BACKSPACE );
  const _0 = new Key( '0', KeyID.ZERO, { horizontalSpan: 2 } );

     PositiveIntegerLayout: [
      [ _7, _8, _9 ],
      [ _4, _5, _6 ],
      [ _1, _2, _3 ],
      [ _0, backspaceKey ]
    ],

    PositiveAndNegativeIntegerLayout: [
      [ _7, _8, _9 ],
      [ _4, _5, _6 ],
      [ _1, _2, _3 ],
      [ backspaceKey, new Key( '0', KeyID.ZERO ), new Key( PLUS_CHAR + '/' + MINUS_CHAR, KeyID.PLUS_MINUS ) ]
    ],

Assigning to @jbphet for initial discussion.

@jbphet
Copy link
Contributor

jbphet commented Dec 17, 2019

Seems like decent idea - it would certainly be easier to read. There is a difference between the code above and the code that is in Keypad.js right now in that the existing code uses unique instances if the Key class in each layout, whereas the suggested code is reusing instances across layouts. I'm not sure if that would have any consequences or not.

Back to @samreid for continued discussion. I don't have time to jump in on this at the moment since it doesn't seem particularly urgent, so you're welcome to try it out, request prioritization, or mark as deferred as you see fit.

@jbphet jbphet assigned samreid and unassigned jbphet Dec 17, 2019
@samreid
Copy link
Member Author

samreid commented Dec 18, 2019

@ariel-phet is there someone else that can look into this? It seems a straightforward suggestion that any developer could investigate.

@ariel-phet
Copy link

marking as medium since should be reasonably straightforward, but not a rush

@Denz1994
Copy link
Contributor

Denz1994 commented Jan 9, 2020

I factored out the keys into constants in KeyPad.js. A few notes about the change:

  • The static layouts use the constants, so the code looks more readable.
  • KeyPad.WierdLayout wasn't refactored because the intent for that layout is for testing (according to its documentation). I thought it would make sense to leave as is since devs will most likely adjust the layout as needed.
  • This was tested by running a few cycles in aqua and testing the keypads in Unit Rates, Projectile Motion, Area-Model-Algebra, and the scenery-phet/components screen.

Note: I did run into a "duplicates not allowed " console error while testing Projectile-Motion but this doesn't seem related to the changes above. Assigning back to @samreid for review.

@Denz1994 Denz1994 assigned samreid and unassigned Denz1994 Jan 9, 2020
@samreid
Copy link
Member Author

samreid commented Jan 9, 2020

The code changes look great, and I tested in scenery-phet test harness. Nice work @Denz1994, thanks, closing.

@samreid samreid closed this as completed Jan 9, 2020
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