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

convert sims to use AquaRadioButtonGroup #555

Open
5 of 25 tasks
pixelzoom opened this issue Jan 3, 2020 · 11 comments
Open
5 of 25 tasks

convert sims to use AquaRadioButtonGroup #555

pixelzoom opened this issue Jan 3, 2020 · 11 comments

Comments

@pixelzoom
Copy link
Contributor

pixelzoom commented Jan 3, 2020

Over in #549, we decided to change how AquaRadioButtons are instrumented. Instead of associating the linked Property with each button, the linked Property will be associated with the group (as is the case for RadioButtonGroup). As noted in #549 (comment):

This also means that all AquaRadioButtons will need to be instantiated via VerticalAquaRadioButtonGroup, not instantiated directly and added to a custom "group" implementation. This is probably a good constraint, since the group needs to support both phet-io and a11y.

So all sims that are explicitly instantiating AquaRadioButtons will need to be changed to use VerticalAquaRadioButtonGroup. This presumably needs to be done immediately for instrumented sims, but could be a "chip away" for non-instrumented sims.

Here's are the lists of sims that are calling new AquaRadioButton, with responsible developers indicated.

Instrumented, or in progress:

Not instrumented:

And perhaps we should add "new AquaRadioButton" to lint rule bad-text.js?

Labeling for dev meeting to discuss.

@zepumph
Copy link
Member

zepumph commented Jan 3, 2020

The PhET-iO team, especially in relation to #550, is wondering if we don't want to try to move AquaRadioButton's functionality into RadioButtonGroup. This would allow us to deprecate VerticalAquaRadioButtonGroup, which is largely duplicated layout code with RadioButtonGroup.

What if there was an option like: stye: 'normal', {'aqua'|'normal'}. Since we may be touching all AquaRadioButton occurrences in this issue anyways, wouldn't it be nicer to end up with one implementation in the end?

More generally rephrasing, the above sentiment: what do we want our long term radio button api to look like (like #523)? Ideally we would have just one implementation under the hood, but we still could have two subtypes if desired.

@pixelzoom
Copy link
Contributor Author

@zepumph I believe that's what will need to happen in order to address #523 (unify radio button group implementations), currently assigned to @chrisklus with low priority.

@pixelzoom
Copy link
Contributor Author

pixelzoom commented Jan 3, 2020

Uh oh, we need to rethink this.

I thought I'd try this conversion to see what problems I might run into. I immediately ran into this situation in acid-base-solutions:

screenshot_41

This is a group of 3 radio buttons. The first radio button has a related checkbox. I think this is a perfectly valid UI design, and I don't think this is an isolated example. And it's impossible to accomplish with VerticalAquaRadioButtonGroup. We could address this by moving the "Molecules" radio button to the bottom of the group. But is that a design constraint that PhET would (or should have to) live with? And what if more than one radio button had an associated checkbox?

hookes-law is similar to acid-base-solutions, where a radio button has an associated UI component. We lucked out in this case because it's associated with the bottom radio button.

screenshot_46

The other type of problem is layout. There are a number of sims that use a horizontal layout, not supported by VerticalAquaRadioButtonGroup. This is an easier problem to solve -- generalize VerticalAquaRadioButtonGroup to AquaRadioButtonGroup with orientation option. Examples:

balancing-chemical-equations:

screenshot_42

beers-law-lab:

screenshot_43

screenshot_44

gravity-and-orbits:

screenshot_45

fluid-pressure-and-flow:

screenshot_47

I said this long ago when VerticalAquaRadioButtonGroup was first created - I don't like it. I don't like the fact that it has responsibility for creating the radio buttons. It should be responsible for the "groupness" and (perhaps) layout. Java's ButtonGroup pattern would be a good place to start: http://www.fredosaurus.com/notes-java/GUI/components/50radio_buttons/25radiobuttons.html

@pixelzoom pixelzoom removed their assignment Jan 3, 2020
@chrisklus
Copy link
Contributor

Should we discuss next steps at an upcoming developer meeting? Perhaps I should be addressing #523 at high priority.

@zepumph
Copy link
Member

zepumph commented Feb 13, 2020

After discussing in the dev meeting today:

  • We don't think it is worth design time on complicated cases until we actually get back to those sims.
  • We could rename VerticalAquaRadioButtonGroup to AquaRadioButtonGroup and add an 'orientation' option, like in RadioButtonGroup. That way a few more cases above could be converted.
  • Still it seemed like there were no objections to converting to this type when appropriate (easy). Adding "chip-away" to this issue.

pixelzoom added a commit that referenced this issue Feb 13, 2020
@pixelzoom pixelzoom self-assigned this Feb 13, 2020
pixelzoom added a commit to phetsims/balloons-and-static-electricity that referenced this issue Feb 13, 2020
pixelzoom added a commit to phetsims/capacitor-lab-basics that referenced this issue Feb 13, 2020
pixelzoom added a commit to phetsims/gravity-and-orbits that referenced this issue Feb 13, 2020
pixelzoom added a commit to phetsims/plinko-probability that referenced this issue Feb 13, 2020
pixelzoom added a commit to phetsims/projectile-motion that referenced this issue Feb 13, 2020
pixelzoom added a commit to phetsims/wave-on-a-string that referenced this issue Feb 13, 2020
pixelzoom added a commit to phetsims/balancing-act that referenced this issue Feb 13, 2020
pixelzoom added a commit to phetsims/pendulum-lab that referenced this issue Feb 13, 2020
pixelzoom added a commit to phetsims/scenery-phet that referenced this issue Feb 13, 2020
pixelzoom added a commit that referenced this issue Feb 13, 2020
@pixelzoom pixelzoom removed their assignment Feb 20, 2020
jbphet added a commit to phetsims/states-of-matter that referenced this issue Mar 2, 2020
jbphet added a commit to phetsims/atomic-interactions that referenced this issue Mar 2, 2020
jbphet added a commit to phetsims/states-of-matter that referenced this issue Mar 2, 2020
@pixelzoom pixelzoom changed the title convert sims to use VerticalAquaRadioButtonGroup convert sims to use AquaRadioButtonGroup Mar 20, 2020
@pixelzoom
Copy link
Contributor Author

In #555 (comment), it was noted that AquaRadioButtonGroup has no support for interleaved UI components, a requirement for several sims. Since work/discussion on this has stalled, I've moved that to its own issue. See #583.

@jbphet
Copy link
Contributor

jbphet commented Mar 26, 2020

This was discussed in the 3/26/2020 developer meeting, and it seems like there is significant tension between the current phet-io API design and the ability to use radio button groups in different layouts, such as the ones shown in #555 (comment). @pixelzoom was making the point that the layout and the "groupness" are separate functionalities that should not be combined into one class, which is where it stands now. We decided that this situation needs to be discussed with the phet-io designers so that we can work out a good way to separate the "groupness" from the layout, yet have it still be usable and not overly complicated in the phet-io API.

I've marked the issue for the next phet-io meeting where @zepumph can bring it up.

@zepumph
Copy link
Member

zepumph commented Jul 16, 2020

From a phet-io spec perspective, the nesting in the studio tree could be pretty flexible.

Option 1 - model structure may be independent of visual display. We would be ok with this, but it isn't quite as clear as Option 3 in the general case.
graphRadioButtonGroup
    barGraphRadioButton
    energyPlotRadioButton
    forcePlotRadioButton
energyCheckbox

Option 2 - nested checkbox is a sibling to its parent radio button (weird). Let's not do this
graphRadioButtonGroup
    barGraphRadioButton
    energyPlotRadioButton
    forcePlotRadioButton
    energyCheckbox

Option 3 - visual nested component would be also nested in studio under that radio button. We prefer this option
graphRadioButtonGroup
    barGraphRadioButton
    energyPlotRadioButton
    forcePlotRadioButton
        energyCheckbox (or any Node)

An important piece that should be implemented for PhET-iO is that toggling the visibility of the parent radio button should also toggle the visibility of the nested content.

Implementation

One way to support this would be in AquaRadioButton solely:
Add support in the items list for a new property, like childContentNode, and then AquaRadioButtonGroup would control the layout of the sub-components

We also discussed solving this more generally, noting that AquaRadioButtons and checkboxes are often stacked, interleaved, and nested (CAF, HL, etc). Perhaps when we work on this next, a layout manager like "VerticalFormTreeLayoutManager" that does this work for any Node tree-like structure.

We decided that we would not work on this until the next time a simulation that uses these weird AquaRadioButton cases would need it. Unassigning until then.

@pixelzoom edit: I modified the above options to correspond to this example in Hooke's Law, since that was the context of our phet-io meeting discussion:

screenshot_46

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

7 participants