phetsims / number-suite-common

"Number Suite Common" contains code for use by sims that are part of the Number Suite
GNU General Public License v3.0
0 stars 0 forks source link

Factor out hard coded string 'NUMBER_COMPARE/' #23

Closed zepumph closed 1 year ago

zepumph commented 1 year ago

Promoting TODO to issue:

https://github.com/phetsims/number-compare/blob/59bf85891fc1e85c4f7462e0c7e1f5c217272427/js/compare/view/ComparisonTextNode.ts#L66

this should be a variable, or so says a TODO.

pixelzoom commented 1 year ago

This constant in ComparisonTextNode.ts:

    // TODO: factor these out somewhere? https://github.com/phetsims/number-suite-common/issues/23
    const numberComparePrefix = 'NUMBER_COMPARE/';

... and this constant in NumberSuiteCommonConstants.ts:

const NUMBER_SUITE_COMMON_STRING_KEY_PREFIX = 'NUMBER_SUITE_COMMON/';

... are both RequireJS namespaces. And it's weird/confusing to have the '/' separator included in the value.

So in the above commits, I added this to NumberCompareConstants.ts:

  // RequireJS namespace, used for looking up translated strings
  NUMBER_COMPARE_REQUIREJS_NAMESPACE: 'NUMBER_COMPARE'

... made this change in NumberSuiteCommonConstants.ts:

// RequireJS namespace, used for looking up translated strings
const NUMBER_SUITE_COMMON_REQUIREJS_NAMESPACE = 'NUMBER_SUITE_COMMON';

... and moved the '/' into the string patterns wherever these constants are used for string lookup. For example, in ComparisonTextNode.ts:

isLessThanString = secondLocaleStrings[ `${NumberCompareConstants.NUMBER_COMPARE_REQUIREJS_NAMESPACE}/isLessThan` ];
isMoreThanString = secondLocaleStrings[ `${NumberCompareConstants.NUMBER_COMPARE_REQUIREJS_NAMESPACE}/isMoreThan` ];
isEqualToString = secondLocaleStrings[ `${NumberCompareConstants.NUMBER_COMPARE_REQUIREJS_NAMESPACE}/isEqualTo` ];

@chrisklus please review.

chrisklus commented 1 year ago

Changes look great, thanks @zepumph and @pixelzoom! Closing.