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

Move backgroundColor from Screen to ScreenView #807

Open
samreid opened this issue May 9, 2022 · 3 comments
Open

Move backgroundColor from Screen to ScreenView #807

samreid opened this issue May 9, 2022 · 3 comments

Comments

@samreid
Copy link
Member

samreid commented May 9, 2022

While working on phetsims/mean-share-and-balance#3, @marlitas and I were surprised to see that the backgroundColor is defined in Screen rather than ScreenView, since it seems to be a view-specific value. We skimmed the usages of background in Joist and could not see a reason that it has to be in the Screen. (It is used to create default homescreen icons, but that seems like it would be OK to be some other color since it is just a placeholder).

It may be a lot of work to move it to the view, since we would have to update 100+ simulation usage sites. We thought it would be good to mention, even if we decide not to implement it.

Assigning to the responsible dev for joist for feedback, but please feel free to escalate to dev meeting if desired.

@zepumph
Copy link
Member

zepumph commented May 9, 2022

Yes, that seems like an oversight to me. It is quite view-like to me. I wonder though if joist needs a more general exploration into its model-view separation. Currently it makes some amount of sense to me to house them on the screen (not that is is best) based on the implementation in Sim.js. What is the view vs model separation in Sim? For example, it has a LookAndFeel which set's its background property based on the screen's Property. If one changed organization, shouldn't the other? For example sim.view.backgroundColorProperty.value = screen.view.backgroundColorProperty.value. Instead of this line:

joist/js/Sim.ts

Line 649 in e0b603f

this.lookAndFeel.backgroundColorProperty.value = Color.toColor( this.screenProperty.value.backgroundColorProperty.value );

@zepumph zepumph assigned samreid and unassigned zepumph May 9, 2022
@samreid
Copy link
Member Author

samreid commented Aug 3, 2022

Raising for dev meeting discussion to determine priority, assignees, timeline and whether it will need to be a chip-away.

@samreid samreid removed their assignment Aug 3, 2022
@chrisklus
Copy link
Contributor

KP: Does this affect phet-io API?
SR: No
KP: This seems like low priority and should probably not be changed right now

Should BackgroundColorProperty be in Screen or ScreenView? We took a vote:

AV: abstain
CM - not present
CK: no strong feelings, would want to look at usages before knowing what feels best
JB - not present
JG - ScreenView. But its used before ScreenViews are constructed currently.
JO - ScreenView preference (if technically possible)
KP- abstain
LM - abstain
LV - abstain
MK screen view
MP - abstain
MS - screen view
SR: I don’t feel too strongly about it, but ScreenView sounds more appropriate I think? But this overlaps with the “what do we mean by “model” question”.
MB: Screen view seems to make sense if color properties for other components are in the view

Leaving unassigned until this is higher priority. Could be done by one person as a chip away.

@samreid samreid changed the title BackgroundColor should be defined in ScreenView, not Screen Move backgroundColor from Screen to ScreenView Dec 9, 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

4 participants