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

Review string reachability and tandems in phet-io #145

Closed
jonathanolson opened this issue Aug 31, 2022 · 9 comments
Closed

Review string reachability and tandems in phet-io #145

jonathanolson opened this issue Aug 31, 2022 · 9 comments
Assignees

Comments

@jonathanolson
Copy link
Contributor

I'm curious if I should be adding tandems to currently un-tandem'ed string "derived" Properties

@arouinfar
Copy link
Contributor

arouinfar commented Mar 14, 2023

@jonathanolson I reviewed Density on master, with a focus on dynamic layout and string discoverability in the Studio tree.

Generally, I expect to see strings related to UI components like sliders and radio buttons linked in the tree, which I see already in Density. I would also expect to see things like panel/AccordionBox titles instrumented/linked too (which I don't). I would not, however, expect to see all of the strings related to the Density Table exposed in the Studio Tree.

Overall, I think Density is looking good. There are a few places where string discoverability could be better.

  • blocksPanel title link to blocksStringProperty (Compare & Mystery screens)
  • densityTableAccordionBox title link to densityTableStringProperty
  • densityAccordionBox title link to densityReadoutStringProperty

@arouinfar
Copy link
Contributor

Tagging for #150

@arouinfar
Copy link
Contributor

@samreid this also looks like an issue you can handle (if time allows).

I reviewed dynamic strings/layout in Density, and it generally all looked good. I found a few places where it would be nice to link the strings to the view. See #145 (comment) for the checklist.

@samreid
Copy link
Member

samreid commented Mar 24, 2023

This issue was still assigned to me from the prior iteration, and I considered unassigning it until a density-focused iteration. But I checked and it seemed straightforward to instrument the Text instances with those strings, so I decided to go for it. @arouinfar or @jonathanolson do not feel obligated to spend time on this if it is not in line with your current priorities.

@samreid
Copy link
Member

samreid commented Mar 24, 2023

The API file is very stale. I'll regenerate it before starting.

samreid added a commit to phetsims/density-buoyancy-common that referenced this issue Mar 24, 2023
@samreid
Copy link
Member

samreid commented Mar 24, 2023

I instrumented the titleText for those 3 checkboxes, @arouinfar can you please review the tandem names and structure?

@arouinfar
Copy link
Contributor

Thanks @samreid!

blocksPanel title link to blocksStringProperty (Compare & Mystery screens)

Looks good for Compare, but was missed on Mystery.

@jonathanolson on the Mystery screen, can you add blocksPanel.titleText and link it back to the blocksStringProperty? Please follow the pattern used on the Compare screen:
image

@jonathanolson
Copy link
Contributor Author

Implemented above, can you verify?

@arouinfar
Copy link
Contributor

Looks good, thanks @jonathanolson!

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