samreid
commented
2 years ago I got very far on this but it is very complicated and I wanted to call for help. Current status:
```diff
Index: main/circuit-construction-kit-common/js/view/ViewRadioButtonGroup.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/main/circuit-construction-kit-common/js/view/ViewRadioButtonGroup.ts b/main/circuit-construction-kit-common/js/view/ViewRadioButtonGroup.ts
--- a/main/circuit-construction-kit-common/js/view/ViewRadioButtonGroup.ts (revision 7bfc9ae148271ef0e9607da3880662fff717a074)
+++ b/main/circuit-construction-kit-common/js/view/ViewRadioButtonGroup.ts (date 1662088624728)
@@ -20,6 +20,7 @@
import CircuitElementViewType from '../model/CircuitElementViewType.js';
import Vertex from '../model/Vertex.js';
import BatteryNode from './BatteryNode.js';
+import { Text } from '../../../scenery/js/imports.js';
// constants
const BATTERY_LENGTH = CCKCConstants.BATTERY_LENGTH;
@@ -70,15 +71,14 @@
isIcon: true,
scale: SCALE
} );
- const lifelikeIcon = createBatteryNode( CircuitElementViewType.LIFELIKE, tandem.createTandem( 'lifelikeIcon' ) );
- const schematicIcon = createBatteryNode( CircuitElementViewType.SCHEMATIC, tandem.createTandem( 'schematicIcon' ) );
super( viewTypeProperty, [ {
value: CircuitElementViewType.LIFELIKE,
- node: lifelikeIcon,
+ createNode: tandem => createBatteryNode( CircuitElementViewType.LIFELIKE, tandem.createTandem( 'lifelikeIcon' ) ),
+ createLabel: tandem => new Text( 'HELLO', { tandem: tandem.createTandem( 'labelNode' ) } ),
tandemName: 'lifelikeRadioButton'
}, {
value: CircuitElementViewType.SCHEMATIC,
- node: schematicIcon,
+ createNode: tandem => createBatteryNode( CircuitElementViewType.SCHEMATIC, tandem.createTandem( 'schematicIcon' ) ),
tandemName: 'schematicRadioButton'
} ], options );
}
Index: main/sun/js/ChildComponentOptions.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/main/sun/js/ChildComponentOptions.ts b/main/sun/js/ChildComponentOptions.ts
--- a/main/sun/js/ChildComponentOptions.ts (revision 287175e0fbb94a6817750468c6b3b4de63fb18d3)
+++ b/main/sun/js/ChildComponentOptions.ts (date 1662088367051)
@@ -28,6 +28,15 @@
tandemName?: string;
};
+export type NodeOrCreateNode = {
+ node: Node;
+ createNode?: never;
+} | {
+ node?: never;
+ createNode: ( tandem: Tandem ) => Node;
+ tandemName?: string;
+};
+
export default ChildComponentOptions;
/**
@@ -35,7 +44,7 @@
* TODO: https://github.com/phetsims/sun/issues/746 should this be renamed? Should it just be a top level `export function`,
* or does it belong elsewhere?
*/
-export function getNodes( array: ChildComponentOptions[], tandem: Tandem ): Node[] {
+export function getNodes( array: NodeOrCreateNode[], tandem: Tandem ): Node[] {
return array.map( item => {
if ( item.node ) {
return item.node;
Index: main/sun/js/buttons/RectangularRadioButtonGroup.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/main/sun/js/buttons/RectangularRadioButtonGroup.ts b/main/sun/js/buttons/RectangularRadioButtonGroup.ts
--- a/main/sun/js/buttons/RectangularRadioButtonGroup.ts (revision 287175e0fbb94a6817750468c6b3b4de63fb18d3)
+++ b/main/sun/js/buttons/RectangularRadioButtonGroup.ts (date 1662088748644)
@@ -22,6 +22,7 @@
import Property from '../../../axon/js/Property.js';
import optionize, { combineOptions } from '../../../phet-core/js/optionize.js';
import StrictOmit from '../../../phet-core/js/types/StrictOmit.js';
+import { getNodes } from '../ChildComponentOptions.js';
// pdom - Unique ID for each instance of RectangularRadioButtonGroup. Used to create the 'name' option that is passed
// to each RectangularRadioButton in the group. All buttons in the group must have the same 'name', and that name
@@ -34,16 +35,29 @@
// Describes one radio button
export type RectangularRadioButtonItem = {
- node: Node; // primary depiction for the button
value: T; // value associated with the button
- label?: Node; // optional label that appears outside the button
phetioDocumentation?: string; // optional documentation for PhET-iO
tandemName?: string; // optional tandem for PhET-iO
tandem?: never; // use tandemName instead of a Tandem instance
labelContent?: PDOMValueType; // optional label for a11y (description and voicing)
voicingContextResponse?: VoicingResponse;
descriptionContent?: PDOMValueType; // optional label for a11y
-};
+} & ( {
+ node: Node;
+ createNode?: never;
+} | {
+ node?: never;
+ createNode: ( tandem: Tandem ) => Node;
+ tandemName: string;
+} ) & ( {
+
+ // optional label that appears outside the button
+ label?: Node;
+ createLabel?: never;
+} | {
+ label?: never;
+ createLabel?: ( tandem: Tandem ) => Node;
+} );
/**
* Identifies a radio button and its layout manager. Pointer areas and focus highlight need to be set on
@@ -104,9 +118,6 @@
'items must have unique values' );
assert && assert( _.find( items, item => ( item.value === property.value ) ),
'one radio button must be associated with property.value' );
- assert && assert( _.every( items, item => !item.node.hasPDOMContent ),
- 'Accessibility is provided by RectangularRadioButton and RectangularRadioButtonItem.labelContent. ' +
- 'Additional PDOM content in the provided Node could break accessibility.' );
const options = optionize()( {
@@ -162,6 +173,11 @@
groupFocusHighlight: true
}, providedOptions );
+ const nodes = getNodes( items, options.tandem );
+ assert && assert( _.every( nodes, node => !node.hasPDOMContent ),
+ 'Accessibility is provided by RectangularRadioButton and RectangularRadioButtonItem.labelContent. ' +
+ 'Additional PDOM content in the provided Node could break accessibility.' );
+
assert && assert( options.soundPlayers === null || options.soundPlayers.length === items.length,
'If soundPlayers is provided, there must be one per radio button.' );
@@ -174,8 +190,8 @@
);
// Calculate the maximum width and height of the content, so we can make all radio buttons the same size.
- const widestContentWidth = _.maxBy( items, item => item.node.width )!.node.width;
- const tallestContentHeight = _.maxBy( items, item => item.node.height )!.node.height;
+ const widestContentWidth = _.maxBy( nodes, node => node.width )!.width;
+ const tallestContentHeight = _.maxBy( nodes, node => node.height )!.height;
// Populated for each radio button in for loop
const buttons: Array | FlowBox> = [];
@@ -187,9 +203,10 @@
for ( let i = 0; i < items.length; i++ ) {
const item = items[ i ];
+ const node = nodes[ i ];
const radioButtonOptions = combineOptions( {
- content: item.node,
+ content: node,
minWidth: widestContentWidth + 2 * xMargin,
minHeight: tallestContentHeight + 2 * yMargin,
soundPlayer: options.soundPlayers ? options.soundPlayers[ i ] :
@@ -229,10 +246,21 @@
radioButton.addChild( boundingRect );
let button;
- if ( item.label ) {
+ if ( item.label || item.createLabel ) {
+
+ // If a label is provided, the button becomes a FlowBox that manages layout of the button and label.
+ const label: Node = ( () => {
+ if ( item.label ) {
+ return item.label;
+ }
+ else if ( item.createLabel ) {
+ return item.createLabel( options.tandem.createTandem( item.tandemName! ).createTandem( 'labelNode' ) );
+ }
+ else {
+ throw new Error( 'illegal state' );
+ }
+ } )();
- // If a label is provided, the button becomes a FlowBox that manages layout of the button and label.
- const label = item.label;
const labelOrientation = ( options.labelAlign === 'bottom' || options.labelAlign === 'top' ) ? 'vertical' : 'horizontal';
const labelChildren = ( options.labelAlign === 'left' || options.labelAlign === 'top' ) ? [ label, radioButton ] : [ radioButton, label ];
button = new FlowBox( {
```
Perhaps after @pixelzoom or @zepumph reviews https://github.com/phetsims/sun/issues/746#issuecomment-1234801112 we can discuss this one?
pixelzoom
zepumph
From https://github.com/phetsims/sun/issues/746#issuecomment-1234801112, @pixelzoom said: