AccordionBox resizing

[jonathanolson]

Per #290 and this coming up as a pain point two consecutive sims I've worked on, I'd like to add a { resize: true } option to AccordionBox, so that it can handle content/title size changes (and potentially other changes) naturally. Default will be to { resize: false } for compatibility.

[jonathanolson]

Assigning to @jbphet for review of changes.

[jbphet]

I have a handful of comments:

• It would be good to add a demo/test of this to the Sun demo, including having a resize button (or something) to resize the content and test/demonstrate this capability. This would be helpful for regression testing future changes as well as for having a demonstration of how to use it.
• this._titleBarFill and this._titleBarStroke are both unused. If the idea is to have all options visible to the methods, why not just add this.options = options? Otherwise, I'd suggest not having unused properties.
• IntelliJ highlights a number of usages of this inside the disposal function. I can see that it is (probably) being set correctly when the disposal function is called, but this approach creates a fair amount of "visual noise" for IntelliJ users. Would it be possible to just use self instead?
• I wanted to test it in action, but couldn't find it being used anywhere (which is another reason to have a demo). AreaScreenView has a TODO about adding it when available, so it would be good to at least add it there and test it out.
• There is a comment that says, "Reverted some of sun#280 changes, doing this BEFORE layout now, since it can happen multiple times". This isn't particularly clear, and would involve going through AccordionBox layout bug #280, understanding it, and figuring out what portions were reverted. Can this be changed to be a standalone comment that describes what's going on and why?

Back to @jonathanolson for actions and discussion.

[jonathanolson]

It would be good to add a demo/test of this to the Sun demo, including having a resize button (or something) to resize the content and test/demonstrate this capability. This would be helpful for regression testing future changes as well as for having a demonstration of how to use it.

Demo added.

this._titleBarFill and this._titleBarStroke are both unused. If the idea is to have all options visible to the methods, why not just add this.options = options? Otherwise, I'd suggest not having unused properties.

Removed the unused properties, sorry about that!

IntelliJ highlights a number of usages of this inside the disposal function. I can see that it is (probably) being set correctly when the disposal function is called, but this approach creates a fair amount of "visual noise" for IntelliJ users. Would it be possible to just use self instead?

Using self now.

I wanted to test it in action, but couldn't find it being used anywhere (which is another reason to have a demo). AreaScreenView has a TODO about adding it when available, so it would be good to at least add it there and test it out.

Pendulum Lab uses it (the energy accordion box changes size on startup, since it's all attached through an AlignGroup). Demo also exists now. Plan to handle that Area Model TODO when doing more Area Model stuff (it's more than just that change).

There is a comment that says, "Reverted some of sun#280 changes, doing this BEFORE layout now, since it can happen multiple times". This isn't particularly clear, and would involve going through #280, understanding it, and figuring out what portions were reverted. Can this be changed to be a standalone comment that describe what's going on and why?

I removed the comment for now, as it doesn't really help the current state of the code. It was basically saying "before we had to do X because Y was broken, but now Y works and now we don't have to do X" with a comment link. That doesn't really help anyone in the future looking at the code (it's just a normal addChild at the normal point), so I think removal of the comment is the right option.

[pixelzoom]

This issue made changes to AccordionBox and is still open. Labeling as "block-publication".

[jbphet]

The commits all look good, and I tested out the demo and it behaves as one would expect. Closing.

[samreid]

In phetsims/least-squares-regression#103, we discovered that "resize: false" sets LayoutConstraint._enabled to false, and hence when the title changes size, it does not recenter. Is this a significant problem? Should something be done to address this? @pixelzoom @jbphet @jonathanolson

[jbphet]

Hard to say without a demo or a bit more information, but my initial thought is that yes, this seems like a problem. It sounds like the title won't correctly reposition when doing things like switching locales, which seems incorrect, and not at all the behavior that I'm generally going for when using resize: false. Seems like an unintended consequence.

[samreid]

You can see the problem in https://phet-dev.colorado.edu/html/least-squares-regression/1.2.0-rc.1/phet/least-squares-regression_all_phet.html?stringTest=dynamic if you use the arrows to extend the strings, the title in the top left accordion box is no longer center aligned. Paper trail is through phetsims/least-squares-regression#103

[jbphet]

Definitely looks a little odd in the example referenced above, but doesn't break the sim or anything. IMO opinion it's worth a bit at least a bit of effort to see if it can be fixed.

Here's an example where it looks a little odd:

[samreid]

This issue was already discovered and under investigation in #890 (comment)

[pixelzoom]

I agree that this would be good to fix. But I don't have any input on priority, scheduling, or assignment.