Skip to content

Commit

Permalink
ComboBox refactor handling REVIEWs, see #95
Browse files Browse the repository at this point in the history
  • Loading branch information
@jonathanolson
jonathanolson committed Mar 10, 2023
1 parent 2f6435f commit e1d161b
Showing 1 changed file with 16 additions and 66 deletions.
82 changes: 16 additions & 66 deletions js/common/view/MySolarSystemComboBox.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@
import TReadOnlyProperty from '../../../../axon/js/TReadOnlyProperty.js';
import LabMode from '../../../../solar-system-common/js/model/LabMode.js';
import SolarSystemCommonConstants from '../../../../solar-system-common/js/SolarSystemCommonConstants.js';
import ComboBox, { ComboBoxItem } from '../../../../sun/js/ComboBox.js';
import ComboBox from '../../../../sun/js/ComboBox.js';
import mySolarSystem from '../../mySolarSystem.js';
import MySolarSystemStrings from '../../MySolarSystemStrings.js';
import MySolarSystemModel from '../model/MySolarSystemModel.js';
Expand All @@ -21,52 +21,7 @@ import { Node, Text } from '../../../../scenery/js/imports.js';
export default class MySolarSystemComboBox extends ComboBox<LabMode> {
public constructor( model: MySolarSystemModel, listParent: Node ) {

//REVIEW: Read this first!
//REVIEW: A Map is being used. We're implicitly relying on the map being iterable in the order that it was
//REVIEW: constructed, which is NOT a guarantee I'd want to rely on.
//REVIEW: I'd suggest the following:
//REVIEW:
//REVIEW: const createItem = ( mode: LabMode, nameProperty: TReadOnlyProperty<string> ) => {
//REVIEW: return {
//REVIEW: value: mode,
//REVIEW: createNode: () => new Text( nameProperty, {
//REVIEW: font: SolarSystemCommonConstants.PANEL_FONT,
//REVIEW: maxWidth: SolarSystemCommonConstants.MAX_WIDTH
//REVIEW: } ),
//REVIEW: a11yName: nameProperty
//REVIEW: };
//REVIEW: };
//REVIEW:
//REVIEW: // ACTUALLY..... I would inline this, so I don't have to name the array
//REVIEW: const comboBoxItems = [
//REVIEW: createItem( LabMode.SUN_PLANET, MySolarSystemStrings.mode.sunAndPlanetStringProperty ),
//REVIEW: createItem( LabMode.SUN_PLANET_MOON, MySolarSystemStrings.mode.sunPlanetAndMoonStringProperty ),
//REVIEW: // ...
//REVIEW: ];
//REVIEW:
//REVIEW: This is a LOT simpler, doesn't require maps, doesn't rely on Map order, or the other issues below.
//REVIEW: BUT: please read about the issues below, hopefully it's helpful!

// Creating pairings between LabMode and their respective string
const modeNameMap = new Map<LabMode, TReadOnlyProperty<string>>();
//REVIEW: We're mapping over an enumeration. Please use EnumerationMap.
//REVIEW: You won't have to guard the map.get() calls below, since it will be guaranteed to have a value.
modeNameMap.set( LabMode.SUN_PLANET, MySolarSystemStrings.mode.sunAndPlanetStringProperty );
modeNameMap.set( LabMode.SUN_PLANET_MOON, MySolarSystemStrings.mode.sunPlanetAndMoonStringProperty );
modeNameMap.set( LabMode.SUN_PLANET_COMET, MySolarSystemStrings.mode.sunPlanetAndCometStringProperty );
modeNameMap.set( LabMode.TROJAN_ASTEROIDS, MySolarSystemStrings.mode.trojanAsteroidsStringProperty );
modeNameMap.set( LabMode.ELLIPSES, MySolarSystemStrings.mode.ellipsesStringProperty );
modeNameMap.set( LabMode.HYPERBOLIC, MySolarSystemStrings.mode.hyperbolicStringProperty );
modeNameMap.set( LabMode.SLINGSHOT, MySolarSystemStrings.mode.slingshotStringProperty );
modeNameMap.set( LabMode.DOUBLE_SLINGSHOT, MySolarSystemStrings.mode.doubleSlingshotStringProperty );
modeNameMap.set( LabMode.BINARY_STAR_PLANET, MySolarSystemStrings.mode.binaryStarPlanetStringProperty );
modeNameMap.set( LabMode.FOUR_STAR_BALLET, MySolarSystemStrings.mode.fourStarBalletStringProperty );
modeNameMap.set( LabMode.DOUBLE_DOUBLE, MySolarSystemStrings.mode.doubleDoubleStringProperty );
modeNameMap.set( LabMode.CUSTOM, MySolarSystemStrings.mode.customStringProperty );

// This creates a ComboBoxItem with the proper a11y content convention
const createModeComboBoxItem = ( mode: LabMode ): ComboBoxItem<LabMode> => {
const nameProperty = modeNameMap.get( mode )!;
const createItem = ( mode: LabMode, nameProperty: TReadOnlyProperty<string> ) => {
return {
value: mode,
createNode: () => new Text( nameProperty, {
Expand All @@ -77,25 +32,20 @@ export default class MySolarSystemComboBox extends ComboBox<LabMode> {
};
};

//REVIEW: 1: const comboBoxItems = [ ...modeNameMap.keys() ].map( createModeNode );
//REVIEW: 2: inline it, since it's only used once
//REVIEW: 3: OR, inline createModeNode. We're just mapping with a function, usually we inline those functions
//REVIEW: 4: You're mapping over modes, which we already have an array for (LabMode.enumeration.values)
//REVIEW: So:
//REVIEW: const comboBoxItems = LabMode.enumeration.values.map( mode => {
//REVIEW: const nameProperty = modeNameMap.get( mode );
//REVIEW: return {
//REVIEW: value: mode,
//REVIEW: createNode: () => new Text( nameProperty, COMBO_BOX_TEXT_OPTIONS ),
//REVIEW: a11yName: nameProperty
//REVIEW: }
//REVIEW: } );
const comboBoxItems: ComboBoxItem<LabMode>[] = [];
modeNameMap.forEach( ( value, key ) => {
comboBoxItems.push( createModeComboBoxItem( key ) );
} );

super( model.labModeProperty, comboBoxItems, listParent, {
super( model.labModeProperty, [
createItem( LabMode.SUN_PLANET, MySolarSystemStrings.mode.sunAndPlanetStringProperty ),
createItem( LabMode.SUN_PLANET_MOON, MySolarSystemStrings.mode.sunPlanetAndMoonStringProperty ),
createItem( LabMode.SUN_PLANET_COMET, MySolarSystemStrings.mode.sunPlanetAndCometStringProperty ),
createItem( LabMode.TROJAN_ASTEROIDS, MySolarSystemStrings.mode.trojanAsteroidsStringProperty ),
createItem( LabMode.ELLIPSES, MySolarSystemStrings.mode.ellipsesStringProperty ),
createItem( LabMode.HYPERBOLIC, MySolarSystemStrings.mode.hyperbolicStringProperty ),
createItem( LabMode.SLINGSHOT, MySolarSystemStrings.mode.slingshotStringProperty ),
createItem( LabMode.DOUBLE_SLINGSHOT, MySolarSystemStrings.mode.doubleSlingshotStringProperty ),
createItem( LabMode.BINARY_STAR_PLANET, MySolarSystemStrings.mode.binaryStarPlanetStringProperty ),
createItem( LabMode.FOUR_STAR_BALLET, MySolarSystemStrings.mode.fourStarBalletStringProperty ),
createItem( LabMode.DOUBLE_DOUBLE, MySolarSystemStrings.mode.doubleDoubleStringProperty ),
createItem( LabMode.CUSTOM, MySolarSystemStrings.mode.customStringProperty )
], listParent, {
//REVIEW: Please provide an options object in the constructor, so these can be provided in the client location
//REVIEW: These options aren't general "all ComboBoxes should have these options" options, they're specific to
//REVIEW: the specific usage, which is in the client (MySolarSystemScreenView)
Expand Down

0 comments on commit e1d161b

Please sign in to comment.