phetsims / circuit-construction-kit-common

"Circuit Construction Kit: Basics" is an educational simulation in HTML5, by PhET Interactive Simulations.
GNU General Public License v3.0
10 stars 10 forks source link

Add PhET-iO customizable text for circuit elements and vertices #812

Closed samreid closed 2 years ago

samreid commented 2 years ago

Related to https://github.com/phetsims/circuit-construction-kit-common/issues/810, add a phet-io customizable text label for each circuit element. Invisible by default. Dynamic layout to make it the right size and position.

SR: Should we make a more generalized engine for creating a label and associating it with a node or position? KP: That has been rarely necessary in other designs. And the label should be part of the studio tree. circuitElementNode.customLabelNode or something, it will always be present in this sim. We also need labels for the vertices.

samreid commented 2 years ago

I added a first draft of putting the custom label text. It seemed most natural to put it with the other CircuitElement model elements, so the tandem is something like:

circuitConstructionKitDc.labScreen.model.circuit.batteryGroup.battery_0.labelTextProperty

If set to the empty string '' it is hidden. It shows in the same panel and same maxWidth as the value display. That part seems ready for review, but I still need to work on the vertex labels.

samreid commented 2 years ago

I added a similar label for vertices. For example:

circuitConstructionKitDc.labScreen.model.circuit.vertexGroup.vertex_0.labelTextProperty

I could use help fine tuning the font sizes and max widths of everything. @arouinfar can you please review and reach out to me for fine tuning?

I also added in Slack:

I added a first draft of custom labels in https://github.com/phetsims/circuit-construction-kit-common/issues/812. The labels know about the CircuitElement but not the CircuitElementNode, so without adding that, we cannot constrain the width based on the circuit element node width. So right now there is a constant maxWidth for each thing. Can you please test and review?

samreid commented 2 years ago

I think this issue is ready for review and feedback. @arouinfar can you please take a look?

samreid commented 2 years ago

@arouinfar and I reviewed this today, and it seems to be working nicely. Closing.

arouinfar commented 2 years ago

@samreid while reviewing #797 I discovered that the labelTextProperty listens to showValuesProperty. This means that a client's custom label will be toggled with the Values checkbox which seems problematic. I think makes sense for the custom label to have the same appearance/styling as the displayed values, but I don't think they should follow the same visibility rules.

I don't think this is the most critical thing to address before delivering the client preview version, but should be addressed for full publication.

EDIT: This only applies to circuit elements, not vertices. The behavior of vertex*.labelTextProperty looks good in master.

arouinfar commented 2 years ago

This looked really buggy when I was doing a walkthrough with the client, so I am going to switch the milestone to the preview version.

samreid commented 2 years ago

Can you please confirm this problem? Testing in master, I see that the label shows over the resistor whether or not the values checkbox is checked:

image
arouinfar commented 2 years ago

Huh, I'm having trouble reproducing, even in the client's dev version. I've definitely run into this a few times, but maybe there's a specific way to trigger it. I'll see what I can do.

arouinfar commented 2 years ago

Ah, I think I've figured it out! I must have pasted the wrong property into my notes when I first ran into this bug. It's not the Values checkbox that's the problem, it's the interaction between isValueDisplayableProperty and labelTextProperty.

Steps to reproduce:

samreid commented 2 years ago

Fixed and ready for testing, please close if all is well.

arouinfar commented 2 years ago

Thanks @samreid, looks good in master!