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 a panel to display % Submerged #112

Closed
DianaTavares opened this issue Mar 1, 2024 · 18 comments
Closed

Add a panel to display % Submerged #112

DianaTavares opened this issue Mar 1, 2024 · 18 comments
Assignees

Comments

@DianaTavares
Copy link

DianaTavares commented Mar 1, 2024

To add this panel, first #32 needs to be solved.

Preference to show/hide this feature in the Simulation Panel
Title: Percent submerged readout
Query Parameter: percentSubmergedReadout
Default true in Basics, Default false in full Buoyancy

image

Here are the mockups of its position in each screen:

Intro
image

Explore
image

Shapes
image

Bottle
Observe how, as this is the only object on the screen, the number is bigger and centered:
image

Boat
Observe that the name in the panel said "% Boat submerged" because in this case, we want to know the percentage of the boat and not the block.

image

@zepumph zepumph mentioned this issue Mar 5, 2024
3 tasks
@zepumph zepumph removed their assignment Mar 11, 2024
AgustinVallejo added a commit to phetsims/density-buoyancy-common that referenced this issue Mar 12, 2024
AgustinVallejo added a commit to phetsims/density-buoyancy-common that referenced this issue Mar 12, 2024
@AgustinVallejo
Copy link
Contributor

AgustinVallejo commented Mar 12, 2024

The above commit improves on this panel. Still missing:

  • Create a visibleProperty and link it to the PreferencesModel
  • Improve the API for Shapes Screen (Handled in Final items for ReadoutAccordionBox density-buoyancy-common#105)
  • Create a parent class for shared functionality with DensityAccordionBox (Tentatively ReadoutListAccordionBox)
  • Don't pass the entire model
  • Create a SetMaterialObject: { material | mass, customName, customFormat }

@AgustinVallejo
Copy link
Contributor

Added to the preferences dialog, noting to change the default to false in DensityBuoyancyCommonQueryParameters before moving on (or move on and remember way down the line, it's okay)

@zepumph
Copy link
Member

zepumph commented Mar 12, 2024

I was just taking a look at the new and improved ReadoutListAccordionBox. I love it! I really like how much we can factor out with setReadout. I believe we can make things much simpler and more suave with a parametric type. If we have a list that can be typesafe, we may be able to move some of the implementation of setReadout to the supertype.

Basic outline:

  • ReadoutListAccordionBox<T> uses CustomReadoutObject which has a list: T[].
  • DensityAccordionBox extends ReadoutListAccordionBox<TReadOnlyProperty<Material>>
  • SubmergedAccordionBox extends ReadoutListAccordionBox<Mass>
  • Then setReadout just uses options.list and we have type safety about it. Not sure how helpful this will be, but let's chat.

I'd just like to think about if this would be helpful. Let's talk about it tomorrow.

@zepumph
Copy link
Member

zepumph commented Mar 12, 2024

  • I'm also wondering if we should have the accordion boxes float up when closed. Probably not, but this view here looks a little awkward:
    localhost_8080_buoyancy_buoyancy_en html_brand=phet ea debugger preferencesStorage

@AgustinVallejo
Copy link
Contributor

@zepumph this question comes up often and I think the answer is always to leave the other components floating there. The same happened multiple times in MSS and Keplers. I would check off that item.

AgustinVallejo added a commit to phetsims/density-buoyancy-common that referenced this issue Mar 13, 2024
AgustinVallejo added a commit to phetsims/density-buoyancy-common that referenced this issue Mar 13, 2024
zepumph added a commit to phetsims/density-buoyancy-common that referenced this issue Mar 13, 2024
Signed-off-by: Michael Kauzmann <michael.kauzmann@colorado.edu>
zepumph added a commit to phetsims/density-buoyancy-common that referenced this issue Mar 13, 2024
…uoyancy#112

Signed-off-by: Michael Kauzmann <michael.kauzmann@colorado.edu>
zepumph added a commit to phetsims/density-buoyancy-common that referenced this issue Mar 13, 2024
Signed-off-by: Michael Kauzmann <michael.kauzmann@colorado.edu>
zepumph added a commit to phetsims/density-buoyancy-common that referenced this issue Mar 13, 2024
Signed-off-by: Michael Kauzmann <michael.kauzmann@colorado.edu>
zepumph added a commit to phetsims/density-buoyancy-common that referenced this issue Mar 13, 2024
Signed-off-by: Michael Kauzmann <michael.kauzmann@colorado.edu>
zepumph added a commit to phetsims/density-buoyancy-common that referenced this issue Mar 13, 2024
Signed-off-by: Michael Kauzmann <michael.kauzmann@colorado.edu>
zepumph added a commit to phetsims/density-buoyancy-common that referenced this issue Mar 13, 2024
Signed-off-by: Michael Kauzmann <michael.kauzmann@colorado.edu>
@zepumph
Copy link
Member

zepumph commented Mar 13, 2024

@zepumph
Copy link
Member

zepumph commented Mar 15, 2024

Please note that I too am making changes to ReadoutAccordionBox over in #96 (AV and I have discussed this, but I'm adding a paper trail).

AgustinVallejo added a commit to phetsims/density-buoyancy-common that referenced this issue Mar 15, 2024
@AgustinVallejo
Copy link
Contributor

This issue has been getting some commits that probably rather belong in phetsims/density-buoyancy-common#105

Assigning this issue back to @DianaTavares to check if the Submerged Accordion Box is ready. In that case, please close.

@zepumph
Copy link
Member

zepumph commented Mar 15, 2024

  • @AgustinVallejo, don't we need to change the default value of the preference for Buoyancy Basics? I see a hard coded value based on the query parameter in DensityBuoyancyCommonPreferences.percentageSubmergedVisibleProperty.

@DianaTavares I believe you can still rework on the review in Buoyancy.

@DianaTavares
Copy link
Author

DianaTavares commented Mar 15, 2024

Cool!
I have some comments:

In the intro:

  • I think is better if these panels are close to the ground (like the mockup)
    image
    image

  • and that the panels to have the same size that in the other screens. This is the picture of Explore:
    image

  • where is the panel in the application screens? In the first message, I included the mockups

  • In Buoyancy (not the basics), the %submerged needs to be by default in the preference as “hide”, and in basics need to be by default close.

@zepumph
Copy link
Member

zepumph commented Mar 18, 2024

I believe that we are having trouble with the new Property for tracking submerged. On CT is seems like we are trying to set it NaN sometimes. Can you take a look?

@zepumph
Copy link
Member

zepumph commented Mar 20, 2024

@DianaTavares Please note that this feature is not working when gravity is 0. See #124

@AgustinVallejo
Copy link
Contributor

AgustinVallejo commented Mar 21, 2024

On our way to close this monster issue. Creating issues for the remaining work:

@phet-dev
Copy link
Contributor

Reopening because there is a TODO marked for this issue.

@zepumph
Copy link
Member

zepumph commented Mar 22, 2024

Ha, we were so close! Unfortunately both of those TODOs seem good to discuss. @AgustinVallejo let's talk about them.

@zepumph zepumph self-assigned this Mar 22, 2024
AgustinVallejo added a commit to phetsims/density-buoyancy-common that referenced this issue Mar 22, 2024
@AgustinVallejo
Copy link
Contributor

Addressed the colorProperty TODO in the above commit, while improving the way those custom explore options were mapped. Left the other one open since I'd like to discuss: As we're returning that pattern conditionally, I'm not sure if creating a PatternStringProperty right there is the right way to go, or would cause memory leaks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants