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

Can phet.chipper.stings.get and string!getStringFromFileContents be simplified? #796

Closed
zepumph opened this issue Sep 23, 2019 · 3 comments
Assignees

Comments

@zepumph
Copy link
Member

zepumph commented Sep 23, 2019

From work with @samreid over in #786, I wonder if these two functions can be combined somehow. These two functions do similar things, but one for requirejs and the other for built mode. I'm especially looking at how both call mapString. Perhaps this could be simplified into a function in ChipperStringUtils.js or something.

@zepumph zepumph self-assigned this Sep 23, 2019
zepumph added a commit that referenced this issue Oct 7, 2019
…ete chipper-strings.js; mapString doesn't need stringTest parameter, #796
@zepumph
Copy link
Member Author

zepumph commented Oct 7, 2019

In the above commit I did the following work on the a11ystring-plugin branch:

  • moved phet.chipper.strings.get to initialize-globals,
  • renamed it to getStringForBuiltSim for clarity
  • deleted chipper-strings.js
  • simplified mapString because it doesn't need stringTest parameter

The goal of these changes was to help with clarity, and to simplify the code that is run between built mode and requirejs. Both call mapString, but before it was pretty confusing how the process for loading the built strings worked. I think in-lining the method previously defined in chipper-strings.js into init globals makes it simpler and nicer. Furthermore I added an assertion to make sure that this method is only ever called when the sim is built.

Note this is all on the branch, because there are too many changes about strings on that branch for me to touch master right now.

@jonathanolson you have the most commits on chipper-strings.js, would you mind reviewing? Does the change to mapString look alright also?

@zepumph zepumph assigned jonathanolson and unassigned zepumph Oct 7, 2019
zepumph added a commit that referenced this issue Oct 7, 2019
@jonathanolson
Copy link
Contributor

I checked over the commit. It looks good, and I like the changes. Separating out the second parameter to a closure variable seems perfect.

@zepumph
Copy link
Member Author

zepumph commented Oct 9, 2019

Excellent! I will close this issue, but it won't be on master for now. see #777

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

No branches or pull requests

2 participants