phetsims / sun

User-interface components for PhET simulations, built on top of Scenery.
MIT License
4 stars 12 forks source link

VerticalCheckboxGroup dynamic layout #835

Closed jonathanolson closed 1 year ago

jonathanolson commented 1 year ago

I saw it computing a static maxItemWidth to use with a strut, so this should presumably NOT work with dynamic layout. This should be updated to a dynamic form.

pixelzoom commented 1 year ago

Yep, was just going to create an issue. From VerticalCheckboxGroup.ts:

    // Determine the max item width
    let maxItemWidth = 0;
    for ( let i = 0; i < nodes.length; i++ ) {
      maxItemWidth = Math.max( maxItemWidth, nodes[ i ].width );
    }

      // Content for the checkbox. Add an invisible strut, so that checkboxes have uniform width.
      const content = new Node( {
        children: [ new HStrut( maxItemWidth ), node ]
      } );
marlitas commented 1 year ago

I'm guessing this will be a similar conversion as AquaRadioButtonGroup. I would like to take a stab at it next week and am adding it to the emergent issues board for my team.

marlitas commented 1 year ago

Commit above addresses this. Definitely went in thinking checkbox hadn't been converted yet, and then questioned wether I was actually understanding the problem.

From my understanding VerticalCheckboxGroup would not dynamically resize when content would become smaller leading to a similar problem found in https://github.com/phetsims/sun/issues/832. I recreated a right align scenario like the one in RAPL to test. Very basic implementation since checkbox is already sizable. Removed hStrut and added a stretch: true. If there is more I'm not understanding please let me know. Otherwise, this is ready for @jonathanolson.

jonathanolson commented 1 year ago

Looks great to me, thanks!