Extreme negative spacing in vertical layout

jonathanolson commented 2 years ago

I kept https://github.com/phetsims/number-line-distance/blob/edd76b95e1650368400657c10bd1a6d68a88eee5/js/common/view/DistanceStatementNode.js#L205 using LayoutBox, since the new FlowBox switch would error out with that.

When I ran it, it was a VBox with -20 spacing, and the top element was 16 pixels high. This means the second element's bounds were actually ABOVE the first element's bounds. This edge case would be quite difficult to handle for advanced layout code (which needs to identify minimum sizes and bounds in a performance-sensitive way), and this was the only case it happened, so I added an assertion in FlowBox to prevent this type of case.

Is that behavior desired? Thoughts? If ideal, it would be best to change up the code to avoid that case (and could use VBox again, which is now backed by FlowBox).

jbphet commented 2 years ago

I think the problematic code is the part that sets negative spacing values. It looks like this:

    this.addChild( new LayoutBox( {
      orientation: 'vertical',
      spacing: options.controlsValues ? -10 : -20, // empirically determined
      children: [
      .
      .
      .

I can see what @SaurabhTotey was trying to do here. He was trying to make the labels at the top of the statement (the x₂ and x₁ in the screen captures below) be very close to the values to which they correspond. To see what I mean, take a look at this screen capture, which is from currently live version, v1.0.2. This uses the negative spacing values.

This one is from master, and I've changed the spacing to zero. As can be seen, the labels are a bit further from the values.

@jonathanolson - I've set the spacing in this LayoutBox to 0 for now with a TODO in the code. In answer to your question "Is that behavior desired?", the answer is yes, kinda. We want to scrunch the labels close the the values, as is shown in the upper one of the two images above, and the negative values for the spacing gave use the desired look. Given the new FlowBox class, what is the best way to achieve this same effect?

jonathanolson commented 2 years ago

The bounds look very expanded for the bottom part!

Could definitely do this with ManualConstraint (in order to vertically position it). I'm not sure how it's written, but it looks like a natural fit for a GridBox layout if you want the e.g. x2 to line up with the 5 vertically.

Would a zoom call to discuss help?

jbphet commented 2 years ago

Something weird happened with my first commit on this issue, possibly a merge that didn't work out right. At any rate, I've just made a commit that sets the spacing to zero and adds a TODO item.

jbphet commented 2 years ago

I was curious about why the labels and the terms weren't sitting right on top of one another when the spacing of the FlowBox was set to zero, and after studying the code for a bit, I can see that it is because there are background nodes behind the terms that are relatively tall. To see these, I (temporarily) added a fill to those nodes. Below are some screenshots followed by the code change that made the backgrounds visible.

    const backgroundNodes = [
      new Rectangle( REPRESENTATION_BOUNDS, { fill: 'rgba( 100, 0, 0, 0.2 )' } ),
      new Rectangle( REPRESENTATION_BOUNDS, { fill: 'rgba( 100, 0, 0, 0.2 )' } )
    ];

Based on this information, I decided to adjust the sizes of these background nodes to see if I could match the previous layout. I was able to get very close, and I'll have the designer review this and decide whether it is close enough.

jbphet commented 2 years ago

@amanda-phet - Some changes to the common code necessitated some changes to this sim, and those changes resulted in subtle differences in the layout, specifically the size of the accordion box that contains the number statement and the positioning of the statement inside the box. The changes are fairly subtle, but I didn't want to assume they were okay without having you take a look, since you were the lead designer. Please check out the current behavior on master and let me know if you feel it is acceptable, and assign this back to me when you're done.

marlitas commented 11 months ago

This was affected a bit more by code changes needed for: https://github.com/phetsims/number-line-distance/issues/75

Will discuss with @amanda-phet as part of some other layout changes that have resulted there.

marlitas commented 11 months ago

Discussed with @amanda-phet and we decided that the current vertical space between the labels and the numbers is okay, we just want the label to be more centered over the number.

amanda-phet commented 11 months ago

Spacing looks good!

phetsims / number-line-distance

Extreme negative spacing in vertical layout #73