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

Convert to new Region and Culture approach #16

Closed
Luisav1 opened this issue Mar 26, 2024 · 5 comments
Closed

Convert to new Region and Culture approach #16

Luisav1 opened this issue Mar 26, 2024 · 5 comments

Comments

@Luisav1
Copy link
Contributor

Luisav1 commented Mar 26, 2024

For phetsims/joist#958 to use localizedImageProperty in Area Model Multiplication and Area Model Algebra.

@Luisav1 Luisav1 self-assigned this Mar 26, 2024
Luisav1 added a commit to phetsims/area-model-algebra that referenced this issue Mar 26, 2024
Luisav1 added a commit to phetsims/area-model-algebra that referenced this issue Mar 26, 2024
Luisav1 added a commit to phetsims/area-model-common that referenced this issue Mar 26, 2024
Luisav1 added a commit to phetsims/area-model-algebra that referenced this issue Mar 26, 2024
Luisav1 added a commit to phetsims/area-model-algebra that referenced this issue Mar 26, 2024
Luisav1 added a commit to phetsims/area-model-common that referenced this issue Mar 26, 2024
Luisav1 added a commit to phetsims/area-model-algebra that referenced this issue Mar 26, 2024
Luisav1 added a commit to phetsims/area-model-common that referenced this issue Mar 26, 2024
@Luisav1
Copy link
Contributor Author

Luisav1 commented Mar 26, 2024

I'm unsure whether to use generic or specific names for image files. Initially, I opted for GameHomeScreenIcon and removed the Variables or Generic prefix to generalize, since they're utilized within their own AMA and AMM sims. However, considering their usage within area-model-common alongside specific elements like VariablesScreen or GenericScreen, I'm unsure about the appropriate naming convention. @pixelzoom Could you provide guidance?

@pixelzoom
Copy link
Contributor

pixelzoom commented Apr 2, 2024

I'm unsure whether to use generic or specific names for image files. ... @pixelzoom Could you provide guidance?

Wow. The classing naming (and documentation) in area-model-common sure is confusing. There are bigger problems here than the image filenames. So I recommend that you just use the existing image filenames, and not try to sort out the naming problem.

I wasn't sure if the R&C conversion was ready for review yet, so I didn't take a look. Feel free to assign back to me for review when you're done.

@Luisav1
Copy link
Contributor Author

Luisav1 commented Apr 2, 2024

Thanks @pixelzoom. I've reverted the renaming commits done above to use the old image filenames. This is now ready for your review.

@Luisav1 Luisav1 assigned pixelzoom and unassigned Luisav1 Apr 2, 2024
pixelzoom added a commit to phetsims/area-model-common that referenced this issue Apr 2, 2024
@pixelzoom
Copy link
Contributor

Conversion looks good.

I found a couple of things in area-model-common that were vestigial (GameScreenIcon, JugglerPortrayal), and deleted them in the above commit. Back to @Luisav1 in case there's anything else that is vestigial that I'm not aware of. Feel free to close if there's nothing else to do.

@Luisav1
Copy link
Contributor Author

Luisav1 commented Apr 3, 2024

Thanks for tidying up the vestigial items in area-model-common. Sorry for missing those files initially, I found no other vestigial items. Closing.

@Luisav1 Luisav1 closed this as completed Apr 3, 2024
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

2 participants