changes to VerticalAquaRadioButtonGroup and VerticalCheckboxGroup

[pixelzoom]

In addition to #262 ...

At the top of VerticalAquaRadioButtonGroup:

//Render a simple vertical checkbox group, where the buttons all have the same sizes
//TODO: not ready for use in simulations, it will need further development & discussion first.
//TODO: Abstract out common functionality between this and VerticalCheckboxGroup
define( function( require ) {

(1) Contrary to the first comment, there are no checkboxes involved here.

(2) Contrary to the first TODO, this module occurs 15 times, in 12 sims, including many that are published.

(3) The second TODO is essentially issue #30, created 10/28/13.

Labeling for discussion at developer meeting.

[zepumph]

It is probably worth reminding people (since I forgot this in the past week) that TODOs in common code should have issues with them.

[jonathanolson]

What priority is handling the TODOs?

[samreid]

@samreid will take a look. It could be that "factor out related parts between vertical checkboxes" is stale, may precede VBox.

I'll also add a comment like // The vertical dilation is set automatically so the buttons are flush

[pixelzoom]

@samreid said:

It could be that "factor out related parts between vertical checkboxes" is stale, may precede VBox.

No longer an issue, see #30.

[pixelzoom]

@samreid I have a little time to kill before my next meeting. I'm going to make a quick "clean up" pass on VerticalAquaRadioButtonGroup and VerticalCheckBoxGroup, then have you review.

[pixelzoom]

First pass of refactoring (rewrite actually) completed. There are many inconsistencies between VerticalAquaRadioButtonGroup and VerticalCheckBoxGroup, see TODO items in VerticalCheckBoxGroup.

[pixelzoom]

Re this TODO in VerticalAquaRadioButtonGroup and VerticalCheckBoxGroup:

//TODO #344 this is the total of left and right margins, replace with xMargin?
padding: 8,

VerticalAquaRadioButtonGroup padding is used in pendulum-lab.
VerticalCheckBoxGroup padding is not used.
So I suggest that we change it to xMargin, for consistency with other PhET UI APIs.

[pixelzoom]

Re this TODO in VerticalCheckboxGroup:

//TODO #344 these are passed to CheckBox, replace with checkboxOptions: {...}
boxWidth: 21,
checkboxColor: 'black',

I suggest that we track down usages of these and change to:

checkboxOptions: {
  boxWidth: 21,
  checkBoxColor: 'black'
}

Re this TODO in VerticalCheckboxGroup:

//TODO #344 item.content is item.node in VerticalAquaRadioButtonGroup

Change content to node.

[pixelzoom]

Re this TODO in VerticalCheckboxGroup:

//TODO #344 VerticalAquaRadioButtonGroup has accessibleLabel, why not here?
//TODO #344 VerticalAquaRadioButtonGroup has a11yNameAttribute, why not here?

@jessegreenberg I suspect that these fields are missing from VerticalCheckBoxGroup because it hasn't been instrumented for a11y yet. Is that correct?

[pixelzoom]

Re these TODOs in VerticalCheckboxGroup:

//TODO #344 indent feature is missing from VerticalAquaRadioButtonGroup, should it be added?
//TODO #344 I can think of other ways to indent that don't involve 2 additional nodes

I think we should add this feature to VerticalAquaRadioButtonGroup. Should be trivial, and then the main for loop in VerticalCheckboxGroup and VerticalAquaRadioButtonGroup would be identical except for the creation of Checkbox vs AquaRadioButton, which would make it easier to address this TODO:

//TODO #344 this for loop looks very much like VerticalAquaRadioButtonGroup, something to factor out?

[pixelzoom]

Re this TODO in

//TODO #344 there's a bug here, indent for each item is not considered

Highly recommended to fix the bug. When indent is used, members of the group currently do not have uniform width. And it's possible (though unlikely) for maxWidth + options.padding - indent to result in a negative HStrut width.

[pixelzoom]

@samreid please review my proposals above for addressing TODO items. Comment here, or ping me on Slack.

[jessegreenberg]

@jessegreenberg I suspect that these fields are missing from VerticalCheckBoxGroup because it hasn't been instrumented for a11y yet.

Correct, VerticalCheckBoxGroup has not yet been instrumented.

[pixelzoom]

a11y-related TODO items removed, a11y instrumentation moved to #349.

[pixelzoom]

To summarize... Here are the proposed changes:

• change items[ {constant:...},...] to items[ {node: ...},...] for VerticalCheckboxGroup
• move option radius into radioButtonOptions for VerticalAquaRadioButtonGroup
• add option checkboxOptions: {...} to VerticalCheckboxGroup
• change option padding to xMargin for VerticalCheckboxGroup and VerticalAquaRadioButtonGroup, with code changes to use 2 * xMargin wherever padding appears
• add indent feature to VerticalAquaRadioButtonGroup
• fix the indent bug in VerticalCheckboxGroup at //TODO #344 there's a bug here...

[pixelzoom]

Unassigning myself. @samreid please reassign to me when you have feedback.

[pixelzoom]

I completed several of the checklist items in #344 (comment). But when I got to the one involving padding, it occurred to me that this responsibility don't belong here. It should be up to whatever is using a group to add margins (x and y!) So my feeling is that the padding option should be removed. Labeling for developer meeting to discuss.

[pixelzoom]

The only place the I see padding: for one of these groups is in PLINKO_PROBABILITY/HopperModeControl, see screenshot below. The padding is really doing nothing here, since there's no container. So I'm going to go ahead and remove the padding option.

[pixelzoom]

The indent option was used in 1 place (WAVE_ON_A_STRING/BottomControlPanel), where it was set to zero. So I see no reason to add it to VerticalAquaRadioButtonGroup, or fix the related bug in VerticalCheckboxGroup. Instead, I'm going to remove the indent option.

[pixelzoom]

All work is completed. Here's a summary of what I did related to #344 (comment):

• VerticalAquaRadioButtonGroup: renamed item.content to item.node
• VerticalAquaRadioButtonGroup: moved option radius into radioButtonOptions
• VerticalCheckboxGroup: added option checkboxOptions
• VerticalCheckboxGroup and VerticalAquaRadioButtonGroup: deleted unused padding option
• VerticalCheckboxGroup: deleted unused item.indent feature

@samreid please review.

[samreid]

The proposed changes look great. I read the combined change sets, the new code, tested Forces and Motion: Basics and Balloons and Static Electricity. I discovered a few tangential issues which I'll reference below, but this issue is ready to close.