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

Mass Control Visibility does not migrate #386

Closed
KatieWoe opened this issue Sep 3, 2024 · 4 comments
Closed

Mass Control Visibility does not migrate #386

KatieWoe opened this issue Sep 3, 2024 · 4 comments
Assignees

Comments

@KatieWoe
Copy link

KatieWoe commented Sep 3, 2024

Device
Samsung
OS
Win 11
Browser
Firefox
Problem Description
For phetsims/qa#1136
If you have density.compareScreen.view.massNumberControl.visibleProperty set to false in Density 1.1, then the control panel will be visible when that file is migrated to 1.2. I haven't yet found other elements that do this, but I'll keep an eye out.
Visuals
massgone
masshere

@KatieWoe KatieWoe added the type:bug Something isn't working label Sep 3, 2024
@KatieWoe
Copy link
Author

KatieWoe commented Sep 4, 2024

density.mysteryScreen.model.blockSets.set1.block1E.visibleProperty and density.mysteryScreen.model.scale.visibleProperty don't migrate either.

@samreid
Copy link
Member

samreid commented Sep 5, 2024

In 1.1, we have:

density.compareScreen.view.massNumberControl
density.compareScreen.view.volumeNumberControl
density.compareScreen.view.densityNumberControl

which are all independent and have independent visibility controls.

However, in 1.2, we have a new container:
density.compareScreen.view.blocksValuePanel

which contains all of the massNumberControl, volumeControl and densityNumberControl.

We could add a rule like "if the user hid all 3 of them in 1.1, then hide blocksValuePanel in 1.2", but what would we want to do if the user only hid 1 or 2 of them?

Let's keep this issue about blocksValuePanel, and move the other problems elsewhere. @zepumph can you please advise?

@zepumph
Copy link
Member

zepumph commented Sep 6, 2024

@samreid and I got this all cleaned up.

Doc in https://github.com/phetsims/phet-io-sim-specific/blob/0742f76f8ae0fd1fcb169607bbb7be545b88b324/repos/density/js/density-migration-processors.ts#L327-L329:

Basically hiding any in the upstream will hide all in the downstream. @KatieWoe will you please review?

@zepumph zepumph assigned samreid and KatieWoe and unassigned zepumph and samreid Sep 6, 2024
zepumph added a commit to phetsims/scenery that referenced this issue Sep 6, 2024
@KatieWoe
Copy link
Author

KatieWoe commented Sep 6, 2024

Things look better to me on main

@KatieWoe KatieWoe assigned zepumph and unassigned KatieWoe Sep 6, 2024
@samreid samreid closed this as completed Sep 7, 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

3 participants