phetsims / circuit-construction-kit-common

"Circuit Construction Kit: Basics" is an educational simulation in HTML5, by PhET Interactive Simulations.
GNU General Public License v3.0
10 stars 10 forks source link

Control panels do not horizontally align correctly when sensor tool icons removed from sensor toolbox #940

Closed matthew-blackman closed 1 year ago

matthew-blackman commented 1 year ago

When the voltmeter or ammeter icons are removed from the sensor toolbox via studio, the control panels do not align correctly. See screenshot below:

Screen Shot 2023-01-23 at 9 22 55 AM

Investigating with the scenery inspector, it looks like the 'advanced' options panel is aligning left within a layout component that is larger than the panel itself. See screenshot below:

Screen Shot 2023-01-23 at 9 25 24 AM
AgustinVallejo commented 1 year ago

Will look into!

matthew-blackman commented 1 year ago

@AgustinVallejo @samreid and I made some progress on this. We found that if titleAlignX is set to 'center' in AdvancedAccordionBox, the panel alignment works but the title text is no longer left aligned. See below:

Screen Shot 2023-01-25 at 1 19 28 PM

This is not an ideal fix but it is getting us closer to the heart of the issue. Additionally, I found that there is an issue with the panel group width if the titleText string is long. See below:

Screen Shot 2023-01-25 at 1 18 33 PM
jonathanolson commented 1 year ago

This looks like something I could help with, might be related to the layout engine?

jonathanolson commented 1 year ago

Should be fixed with the above commits. It seems like code was added that changed the collapsedBox's bounds AFTER we used those bounds for the workaroundBox.

I also patched a case where the rectBounds of the workaroundBox would not have been getting properly updated.

samreid commented 1 year ago

Excellent, thanks @jonathanolson!

matthew-blackman commented 1 year ago

Things are looking great @jonathanolson thanks for looking into this!

One remaining issue is the width of the advanced accordion box when the title text is long. This is likely a CCK issue that I can look into. See below:

Screen Shot 2023-01-26 at 10 32 14 AM
samreid commented 1 year ago

Perhaps try to find a way to reduce the maxWidth of that accordion box title/label?

marlitas commented 1 year ago

https://github.com/phetsims/sun/issues/803 may be related to this issue.

samreid commented 1 year ago

I believe the best long-term solution for this issue may involve a resizable accordion box, as described in https://github.com/phetsims/sun/issues/803. For this release, it seems a very minor aesthetic issue in a corner case, and perhaps we should consider that, like related issue https://github.com/phetsims/circuit-construction-kit-common/issues/939#issuecomment-1416672382, this should not be a necessary feature for publishing this release.

@arouinfar OK to remove this issue from the milestone?

arouinfar commented 1 year ago

it seems a very minor aesthetic issue in a corner case

I agree. The behavior has improved since originally reported, and the remaining issue really seems more like an edge case tom.

OK to remove this issue from the milestone?

Yes, untagging it now.

samreid commented 1 year ago

In today's meeting, @arouinfar @matthew-blackman and I agreed that this issue can be closed, because it is a minor cosmetic issue for this sim, and cannot be implemented until the accordion box is resizable in https://github.com/phetsims/sun/issues/803. We suspect another sim will bump the priority of that issue, then we can re-evaluate in other sims as necessary.