phetsims / scenery

Scenery is an HTML5 scene graph.
http://scenerystack.org/
MIT License
55 stars 12 forks source link

layoutOptions is not working as expected for Separator. #1626

Open pixelzoom opened 7 months ago

pixelzoom commented 7 months ago

For https://github.com/phetsims/graphing-lines/issues/149, EquationAccordionBox has an HSeparator at the top of its content that is intended to be visible, for example in the published version:

screenshot_3168

Code for the content in EquationAccordionBox.ts:

    const contentNode = new VBox( {
      align: 'center',
      spacing: 10,
      children: [
        new HSeparator( {
          stroke: SEPARATOR_STROKE
        } ),
        interactiveEquationNode,
        new HSeparator( {
          stroke: SEPARATOR_STROKE
        } ),
        buttonGroup
      ]
    } );

In main, the top HSeparator is not visible. VBox/Flowbox, makes a separator invisible if there is not a visible Node on each side of the separator -- and this is expected.

But I also expected to be able to set isSeparator: false to make the top separator visible. This did not work -- there is no visible HSeparator:

         new HSeparator( {
-          stroke: SEPARATOR_STROKE
+          stroke: SEPARATOR_STROKE,
+          layoutOptions: {
+            isSeparator: false
+          }
         } ),

When I added the additional stretch: true, the HSeparator became visible:

        new HSeparator( {
          stroke: SEPARATOR_STROKE
          layoutOptions: {
-           isSeparator: false
+           isSeparator: false,
+           stretch: true
          }
        } ),

So there's something funny going on here with nested layoutOptions. In Separator.ts, DEFAULT_SEPARATOR_LAYOUT_OPTIONS was added by @AgustinVallejo in https://github.com/phetsims/scenery/commit/e078be40455e51b88b191ae44807949441aea97a, and its use looks suspicious, relevant code below. Perhaps DEFAULT_SEPARATOR_LAYOUT_OPTIONS is getting overwritten? Or optionize is not working correctly for nested options?

export const DEFAULT_SEPARATOR_LAYOUT_OPTIONS = {
  stretch: true,
  isSeparator: true
};

export default class Separator extends Line {
  public constructor( providedOptions?: SeparatorOptions ) {
    super( optionize<SeparatorOptions, SelfOptions, LineOptions>()( {
      layoutOptions: DEFAULT_SEPARATOR_LAYOUT_OPTIONS,

Assigning to @AgustinVallejo and @jonathanolson.

jonathanolson commented 7 months ago

Looks like this should be an optionize3, {}, DEFAULT_SEPARATOR_LAYOUT_OPTIONS, and then the rest.

jonathanolson commented 7 months ago

Fixed above, @pixelzoom can you review?

pixelzoom commented 7 months ago

This patch shows that this is not fixed. The accordion box shown in https://github.com/phetsims/scenery/issues/1626#issue-2221581677 has no visible HSeparator at the top unless I also add stretch: true.

Subject: [PATCH] rename PumpHandleDragListener for consistency with PumpHandleNode, https://github.com/phetsims/scenery-phet/issues/848
---
Index: js/common/view/EquationAccordionBox.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/common/view/EquationAccordionBox.ts b/js/common/view/EquationAccordionBox.ts
--- a/js/common/view/EquationAccordionBox.ts    (revision 3597139b6d07b0840c8c1c3e69276123631422b5)
+++ b/js/common/view/EquationAccordionBox.ts    (date 1712252607308)
@@ -84,8 +84,7 @@
         new HSeparator( {
           stroke: SEPARATOR_STROKE,
           layoutOptions: {
-            isSeparator: false,
-            stretch: true
+            isSeparator: false
           }
         } ),
         interactiveEquationNode,