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

Updates to preference for percentageSubmergedVisible #126

Closed
zepumph opened this issue Mar 21, 2024 · 6 comments
Closed

Updates to preference for percentageSubmergedVisible #126

zepumph opened this issue Mar 21, 2024 · 6 comments

Comments

@zepumph
Copy link
Member

zepumph commented Mar 21, 2024

From #112. We need to be able to toggle the default of this preference based on the sim running. In Buoyancy we want the preference to default off (not showing the panel), then in buoyancy basics we do want it by default. The query parameter should still override these defaults though, so we many need to outfit a null/true/false value so we know if the query parameter was provided. Another idea is to just use QueryStringMachine.containsKey() to know if the value was provided vs is the default.

@samreid
Copy link
Member

samreid commented Apr 10, 2024

I'll take a look at this. Self-assigning.

@samreid samreid assigned samreid and unassigned zepumph and AgustinVallejo Apr 10, 2024
samreid added a commit to phetsims/buoyancy-basics that referenced this issue Apr 10, 2024
@samreid
Copy link
Member

samreid commented Apr 10, 2024

I recommend a solution like this:

Subject: [PATCH] Address TODOs, see https://github.com/phetsims/chipper/issues/1429
---
Index: js/common/DensityBuoyancyCommonQueryParameters.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/common/DensityBuoyancyCommonQueryParameters.ts b/js/common/DensityBuoyancyCommonQueryParameters.ts
--- a/js/common/DensityBuoyancyCommonQueryParameters.ts	(revision 0c8d369951cb271e2ae1464488db56a95af5f0c2)
+++ b/js/common/DensityBuoyancyCommonQueryParameters.ts	(date 1712764806826)
@@ -11,6 +11,8 @@
 export const VolumeUnitsValues = [ 'liters', 'decimetersCubed' ] as const;
 export type VolumeUnits = ( typeof VolumeUnitsValues )[number];
 
+const defaultPercentageSubmergedVisible = phet.joist.packageJSON.name !== 'buoyancy-basics';
+
 const DensityBuoyancyCommonQueryParameters = QueryStringMachine.getAll( {
 
   gEarth: {
@@ -31,7 +33,7 @@
   // Displays/hides the percentage submerged readout accordion box
   percentageSubmergedVisible: {
     type: 'boolean',
-    defaultValue: true,
+    defaultValue: defaultPercentageSubmergedVisible,
     public: true
   },
 

@zepumph does this look good? Feel free to commit or let me know if I should.

@samreid samreid assigned zepumph and unassigned samreid Apr 10, 2024
samreid added a commit to phetsims/density-buoyancy-common that referenced this issue Apr 10, 2024
samreid added a commit to phetsims/density-buoyancy-common that referenced this issue Apr 10, 2024
@zepumph
Copy link
Member Author

zepumph commented Apr 10, 2024

@samreid and I got to a commit point. Using packageJSON.name was such a nicer way to handle this than anything I was going to recommend. We also cleaned up how the preference is managed, since it was added to density too.

@DianaTavares, the behavior is now as follows:

  • In Density, there is no notion of % submerged, (preference or panels)
  • In Buoyancy, % submerged is supported, but defaults off.
  • In Buoyancy Basics, % submerged is supported, but defaults on.

Please review and feel free to close.

@zepumph zepumph closed this as completed Apr 10, 2024
@zepumph zepumph reopened this Apr 10, 2024
@zepumph zepumph assigned samreid and DianaTavares and unassigned zepumph and samreid Apr 10, 2024
@DianaTavares
Copy link

Thanks team!

The behavior is correct! But in Buoyancy basics, yes the panel is on in the preference menu, but

  • the panel needs to be closed by default.

Like this:
image

Basics don't have the “compare” screen, but there also the % submerged panel is on, but close by default.

@zepumph
Copy link
Member Author

zepumph commented Apr 12, 2024

Currently we don't actually have any code implemented for buoyancy basics. This is just an exact copy of the buoyancy explore screen. I wonder if we can add this to a list for when we implement the buoyancy basics sim.

@zepumph
Copy link
Member Author

zepumph commented Apr 12, 2024

Wait. In Buoyancy, should the panel default to closed also?

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