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

Add "joist" to bad text in multiple common code repos #1004

Closed
zepumph opened this issue Jan 22, 2021 · 4 comments
Closed

Add "joist" to bad text in multiple common code repos #1004

zepumph opened this issue Jan 22, 2021 · 4 comments

Comments

@zepumph
Copy link
Member

zepumph commented Jan 22, 2021

From discussion around phetsims/tambo#126 (comment).

This will likely have to wait on a fair number of issues to complete that decouples things first:

phetsims/tambo#126 (comment)
phetsims/joist#685
phetsims/joist#686
(insert issues about passing phet.joist.display to TAMBO/DisplayedProperty)

on-hold until then.

@zepumph zepumph self-assigned this Jan 22, 2021
@zepumph
Copy link
Member Author

zepumph commented Apr 10, 2023

I finally got around to this today. It looks like there were about 5 places that could have failed if run outside of phetsims, and I was able to clean up other usages, exchanging hasIn with get.

The best part of this for me is the new lint config called phet-library_eslintrc.js, this is a spot that we can use to create failure points when common libraries don't adhere to the open-source conventions we decide on. @samreid will you please review, especially in ReadOnlyProperty and AnimatedPanZoomListener.

@samreid
Copy link
Member

samreid commented Apr 11, 2023

I reviewed the commits and this is a good improvement, thanks! It made me wonder if we want to move these joistlike model attributes like localeProperty and isSettingPhetioStateProperty to a library that exists for common code. Perhaps in axon or maybe isSettingPhetioStateProperty would be in Tandem? What do you think?

@zepumph
Copy link
Member Author

zepumph commented Apr 17, 2023

I could see that as a reasonable path forward, though I do not think that it is worth it for the i18n stuff. In part I like code that guards around this gracefully, because that helps in understanding that this part of a common code element is actually a phet-sim-specific implementation, and most likely not important to the component itself for more general use. I created phetsims/tandem#294, any other thoughts?

@zepumph zepumph assigned samreid and unassigned zepumph Apr 17, 2023
@samreid
Copy link
Member

samreid commented Apr 18, 2023

That sounds reasonable to me, ready to close?

@samreid samreid assigned zepumph and unassigned samreid Apr 18, 2023
@zepumph zepumph closed this as completed Apr 18, 2023
zepumph added a commit to phetsims/perennial that referenced this issue Oct 22, 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