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

AccordionBox does not handle dynamic layout of title. #890

Open
pixelzoom opened this issue Aug 1, 2024 · 7 comments
Open

AccordionBox does not handle dynamic layout of title. #890

pixelzoom opened this issue Aug 1, 2024 · 7 comments
Assignees
Labels

Comments

@pixelzoom
Copy link
Contributor

For example, here's what I see in BCE with ?stringTest=dynamic. The titles should remain centered. The relevant code in BCE is BoxNode, a subclass of AccordionBox.

screenshot_3448
@pixelzoom
Copy link
Contributor Author

pixelzoom commented Aug 1, 2024

Here's the relevant code in AccordionBox.ts. Note that there's no listener for this.titleNode.localBoundsProperty, so no dynamic layout.

    // title horizontal layout
    if ( isWidthSizable( this.titleNode ) ) {
      this.titleNode.preferredWidth = titleRightSpan - titleLeftSpan;
    }
    if ( options.titleAlignX === 'left' ) {
      this.titleNode.left = titleLeftSpan;
    }
    else if ( options.titleAlignX === 'right' ) {
      this.titleNode.right = titleRightSpan;
    }
    else { // center
      this.titleNode.centerX = collapsedBounds.centerX;
    }

    // button & title vertical layout
    if ( options.titleAlignY === 'top' ) {
      this.expandCollapseButton.top = this.collapsedBox.top + Math.max( options.buttonYMargin, options.titleYMargin );
      this.titleNode.top = this.expandCollapseButton.top;
    }
    else { // center
      this.expandCollapseButton.centerY = this.collapsedBox.centerY;
      this.titleNode.centerY = this.expandCollapseButton.centerY;
    }

@jonathanolson
Copy link
Contributor

Weird, adding the titleNode to the LayoutConstraint with addNode (

this.addNode( titleNode );
) SHOULD be adding a listener to title bounds changes. Investigating why this isn't working.

@jonathanolson
Copy link
Contributor

jonathanolson commented Aug 1, 2024

Ahh, resize: false provided in BoxNode is turning layout updates off for the component. Presumably in this case, that can be removed?

https://github.com/phetsims/balancing-chemical-equations/blob/88c42d33a666b9e8d16bc09162699a4242d05356/js/common/view/BoxNode.ts#L67

@pixelzoom
Copy link
Contributor Author

pixelzoom commented Aug 7, 2024

Ahh, resize: false provided in BoxNode is turning layout updates off for the component. Presumably in this case, that can be removed?

Will do for BoxNode.

But in general... Why does resize: false affect alignment of the title? If the title gets smaller, or its maxWidth is set appropriately (as is the case with BoxNode), there would be no need to resize the AccordionBox.

@jonathanolson
Copy link
Contributor

But in general... Why does resize: false affect alignment of the title?

Because the layout code is currently responsible for aligning the title, and resize:false is currently used to determine whether we run the layout more than one time.

This seems a bit conceptually wrong, since the title positioning (and potentially other parts of the layout) aren't technically related to resizing.

@jonathanolson
Copy link
Contributor

We could wrap the titleNode within an AlignBox that is sized to the space it has (determined during layout), so that it will correct alignment when not resizing. Do you think that would be helpful?

@pixelzoom
Copy link
Contributor Author

Yes.

@pixelzoom pixelzoom assigned jonathanolson and unassigned pixelzoom Aug 19, 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