-
Notifications
You must be signed in to change notification settings - Fork 12
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
AccordionBox does not support rendering nodes in titleNode area when opened. #843
Comments
See also related issue #803 |
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) |
@jonathanolson everything is looking great, thanks! We tried passing 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: 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? |
Sorry for the delayed review here. The logic in d17cdcf look appropriate, and this has also been tested by QA with the publication of CAV. I believe this issue can be closed. Thanks! |
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.
This is currently not possible with accordion box, and results in the following:
This line of code was used to create a hacky solution:
@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 #803, or is it a much separate lift?
The text was updated successfully, but these errors were encountered: