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

Clean up Text releated IOTypes #1008

Closed
zepumph opened this issue Nov 4, 2019 · 3 comments
Closed

Clean up Text releated IOTypes #1008

zepumph opened this issue Nov 4, 2019 · 3 comments
Assignees

Comments

@zepumph
Copy link
Member

zepumph commented Nov 4, 2019

From phetsims/gravity-force-lab#183. We have RichTextIO and TextIO, and I see a few issues:

  • Why do we need both of these, couldn't both be TextIO, and we can have one public interface for text in PhET-iO that all core types can adhere to?
  • TextIO has many methods that I feel like are unneeded. These are about maxWidth and font options. Can we remove this?
  • If we create a single TextIO type, we should make sure that the textProperty by default is phetioState:true.

Tagging @samreid and @chrisklus for comment.

@zepumph
Copy link
Member Author

zepumph commented Nov 4, 2019

For the direct fix, I'm going to swap RichTextIO.textProperty to, by default, be phetioState: true.

@samreid
Copy link
Member

samreid commented Nov 4, 2019

It seems we could get pretty far by using one IO type for both of these. I don't understand all the public methods in RichText.js, but one day if we need to support them, we could make RichTextIO extends TextIO.

@zepumph zepumph self-assigned this Nov 4, 2019
@samreid samreid removed their assignment Jun 11, 2020
@zepumph
Copy link
Member Author

zepumph commented Oct 14, 2022

  • Why do we need both of these, couldn't both be TextIO, and we can have one public interface for text in PhET-iO that all core types can adhere to?

This is not the direction we have generally gone for IOTypes in the past 3 years. @samreid would you like a single IOType for Text? Or is it beneficial to split them out because then they match the core type? since Text's public API will not every be more than a subset of RichText, I don't care too much, and it is probably best as it is.

  • TextIO has many methods that I feel like are unneeded. These are about maxWidth and font options. Can we remove this?

These have been removed

In general this issue is out dated. I'm ready to close. Please reopen if you feel differently @samreid.

@zepumph zepumph closed this as completed Oct 14, 2022
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

3 participants