Skip to content
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

layoutOptions is not working as expected for Separator. #1626

Open
pixelzoom opened this issue Apr 2, 2024 · 3 comments
Open

layoutOptions is not working as expected for Separator. #1626

pixelzoom opened this issue Apr 2, 2024 · 3 comments
Assignees
Labels

Comments

@pixelzoom
Copy link
Contributor

pixelzoom commented Apr 2, 2024

For phetsims/graphing-lines#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 e078be4, 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
Copy link
Contributor

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

@jonathanolson
Copy link
Contributor

Fixed above, @pixelzoom can you review?

@pixelzoom
Copy link
Contributor Author

pixelzoom commented Apr 4, 2024

This patch shows that this is not fixed. The accordion box shown in #1626 (comment) 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,

@pixelzoom pixelzoom assigned jonathanolson and unassigned pixelzoom Apr 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants