phetsims / sun

User-interface components for PhET simulations, built on top of Scenery.
MIT License
4 stars 12 forks source link

AccordionBox does not support rendering nodes in titleNode area when opened. #843

Closed marlitas closed 5 months ago

marlitas commented 1 year ago

For Center and Variability, the design calls for an info button, and dot plot renderings to be shown on top of the titleNode area when the accordion box is open. image

This is currently not possible with accordion box, and results in the following:

image

This line of code was used to create a hacky solution:

    backgroundNode.localBounds = backgroundNode.localBounds.copy();

@samreid and I feel like it would be best to remove this line and allow accordionBox to properly apply the desired layout. @jonathanolson, does this feel like something that can be part of work for https://github.com/phetsims/sun/issues/803, or is it a much separate lift?

samreid commented 1 year ago

Likewise, here is a design mock-up calling for data points to render up past the title bar:

image
samreid commented 1 year ago

See also related issue https://github.com/phetsims/sun/issues/803

jonathanolson commented 1 year ago

AccordionBox now supports width-sizable titleNode elements, so you could now HBox something if you want it right-aligned (note that titleAlignX would put whitespace at the right side if it's 'center' due to its design, use either other align)

samreid commented 1 year ago

@jonathanolson everything is looking great, thanks! We tried passing showTitleWhenExpanded: false, and saw that the info button and data could go up into the title region:

image

We would additionally like to show the title anyways (even though it might overlap with some content), and have the title bar be pickable. This patch hard codes some of that behavior to show what we mean:

```diff Subject: [PATCH] Add tab traversal for top soccer ball nodes, see https://github.com/phetsims/center-and-variability/issues/162 --- Index: main/center-and-variability/js/variability/view/VariabilityAccordionBox.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/main/center-and-variability/js/variability/view/VariabilityAccordionBox.ts b/main/center-and-variability/js/variability/view/VariabilityAccordionBox.ts --- a/main/center-and-variability/js/variability/view/VariabilityAccordionBox.ts (revision ccfef71c8827dc74958b20817614a36cf1e15946) +++ b/main/center-and-variability/js/variability/view/VariabilityAccordionBox.ts (date 1684335975469) @@ -30,9 +30,12 @@ public constructor( model: VariabilityModel, layoutBounds: Bounds2, tandem: Tandem, top: number ) { const backgroundShape = CAVConstants.ACCORDION_BOX_CONTENTS_SHAPE_VARIABILITY; - const backgroundNode = new Path( backgroundShape, { - clipArea: backgroundShape, - fill: null + const backgroundNode = new Node( { + children: [ new Path( backgroundShape, { + clipArea: backgroundShape, + fill: null, + pickable: false + } ) ] } ); const currentProperty = new DerivedProperty( [ model.selectedVariabilityMeasureProperty ], selectedVariability => Index: main/center-and-variability/js/common/view/CAVAccordionBox.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/main/center-and-variability/js/common/view/CAVAccordionBox.ts b/main/center-and-variability/js/common/view/CAVAccordionBox.ts --- a/main/center-and-variability/js/common/view/CAVAccordionBox.ts (revision ccfef71c8827dc74958b20817614a36cf1e15946) +++ b/main/center-and-variability/js/common/view/CAVAccordionBox.ts (date 1684336030812) @@ -44,6 +44,7 @@ expandCollapseButtonOptions: { sideLength: BUTTON_SIDE_LENGTH }, + showTitleWhenExpanded: false, // TODO: This is currently highlighting a layout issues with AccordionBox, see: https://github.com/phetsims/center-and-variability/issues/170 titleBarOptions: { stroke: 'black', Index: main/sun/js/AccordionBox.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/main/sun/js/AccordionBox.ts b/main/sun/js/AccordionBox.ts --- a/main/sun/js/AccordionBox.ts (revision 2e21515aea45cfbb3b6876bf132e96b2faff4d00) +++ b/main/sun/js/AccordionBox.ts (date 1684336224682) @@ -125,7 +125,7 @@ private readonly collapsedBoxOutline: Rectangle | null = null; private readonly constraint: AccordionBoxConstraint; - + public static readonly AccordionBoxIO = new IOType( 'AccordionBoxIO', { valueType: AccordionBox, supertype: Node.NodeIO, @@ -302,7 +302,7 @@ } // Set the input listeners for the expandedTitleBar - if ( options.showTitleWhenExpanded && options.titleBarExpandCollapse ) { + if ( options.titleBarExpandCollapse ) { this.expandedTitleBar.addInputListener( { down: () => { if ( this.expandCollapseButton.isEnabled() ) { @@ -413,7 +413,7 @@ this.expandedBox.visible = expanded; this.collapsedBox.visible = !expanded; - titleNode.visible = ( expanded && options.showTitleWhenExpanded ) || !expanded; + titleNode.visible = true; pdomContainerNode.setPDOMAttribute( 'aria-hidden', !expanded ); ```

@jonathanolson what do you think? Should we add support for features like that?

marlitas commented 5 months ago

Sorry for the delayed review here. The logic in https://github.com/phetsims/sun/commit/d17cdcf987697d60ccbfc13bd9ab1096c355197a look appropriate, and this has also been tested by QA with the publication of CAV. I believe this issue can be closed. Thanks!