Closed marlitas closed 6 months ago
@Luisav1 and I worked on this patch together that seems to solve the problem. We checked the before and after on arithmetic, BCE, area-model-algebra/multiplication, and vegas demos. We saw no changes here. There is still work to be done to address all uses of LevelSelectionButtonGroup
to make sure options are updated.
@Luisav1 mentioned she can keep working on using the new options to size buttons and make sure all buttons remain a consistent size.
@pixelzoom your review and input here would be greatly appreciated. Let me know if you would like to connect synchronously.
The commits above include the changes in the patch and also implement the new options in:
I checked their before and after to ensure the button sizes remained the same. I also checked Equality Explorer and Number Play as they use LevelSelectionButtonGroup
too but nothing changed because they use the default button size.
@marlitas said:
@pixelzoom your review and input here would be greatly appreciated. Let me know if you would like to connect synchronously.
I have a pile of high priority things I'm wading through. So it would be best to assign this to me, and/or schedule a time to chat on Zoom.
Now that this is committed, I'll go ahead and assign to you as ready for review. This is currently not blocking for us and I imagine won't become a higher priority until next week at the earliest.
@marlitas and I discussed this on Zoom.
The changes made so far are OK.
The remaining problems seem due to the fact that ButtonNodeConstraint (in ButtonNode) assume that content
is all that it needs to be concerned about, and LevelSelectionButton decorates RectangularPushButton vis this.addChild( bestTimeNode )
. This is a problem that I see in other UI components that have "content" - Panel, AccordionBox,... If addChild
is not going to be supported by layout, then addChild
should not be allowed.
To address LevelSelectionButton specifically, I would consider adding bestTimeNode
as a child of content
, rather than as a child of LevelSelectionButton. I would also clean up LevelSelectionButton to use VBox, etc. for layout, rather than the current implementation that relies on specifically handling translations for all of the subcomponents.
I started to do some work here to address this issue, but the more I got into it the more it felt like I need to wait to design decisions are made in the design meeting next week. Below is a partial patch for if we decide to go the VBox route that keeps the current design intact, but I'm very much hoping we put the bestTimeNode inside the button instead.
@marlitas While you're dealing with layout of LevelSelectionButtonGroup ...
It would be preferrable to use GridBox instead of the current FlowBox. Sims that want need than one row of buttons currently need to resort to a hack, like this example in LineGameScreenView.ts for graphing-lines:
flowBoxOptions: {
...
preferredWidth: 800, // set empirically to provide 3 buttons per row
wrap: true,
...
}
It would be so much nicer (and more robust!) to do:
gridBoxOptions: {
columns: 3
}
It would be preferrable to use GridBox instead of the current FlowBox.
Completely agree. I'll add that into the work being done here. Should be easy to implement.
Meeting 12/15/23
Time button layout:
In the end we decided to remove any notion of time from the LevelSelectionScreen. Therefore we no longer need to worry about this in terms of layout.
In the end we decided to remove any notion of time from the LevelSelectionScreen. Therefore we no longer need to worry about this in terms of layout.
Clarification -- we decided to delete the time associated with each LevelSelectionButton. We did not decide to remove the optional time (clock) toggle button.
bestTime has been removed with the above commits. Over to @pixelzoom for review.
Changes to LevelSelectionButton look good.
demoLevelSelectionButton and demoLevelSelectionButtonGroup were left with a lot of vestigial stuff related to best time. I cleaned things up in https://github.com/phetsims/vegas/commit/df37c819847f068846c663caf737f12430e0f0ea. Back to @marlitas to review, close if OK.
Thanks for catching that @pixelzoom. Looks good to me. Closing!
Reopening because there is a TODO marked for this issue.
@marlitas This issue was automatically reopened because there is a TODO referencing this issue in arithmetic LevelSelectionNode.js.
Ah yes. Thank you phet-dev bot. Removed above.
Reopening. The commit above (https://github.com/phetsims/phet-io-client-guides/commit/fb58a5cfb61bda5fcdd60aa696df1007e86a0543) put the energy-forms-and-changes 1.4 release branch into a broken state.
Looks like energy-forms-and-changes had a maintenance update on December 11th, but then December 15th or some time later, it looks like the commit was added into the energy-forms-and-changes-1.4 branch (https://github.com/phetsims/phet-io-client-guides/commits/energy-forms-and-changes-1.4/), but the dependencies.json was not updated in energy-forms-and-changes 1.4 branch (https://github.com/phetsims/energy-forms-and-changes/blob/1.4/dependencies.json).
Did the commit mean to hit the release branch? It's blocking my pending maintenance release.
Sorry, I have no insight into why that commit was picked to energy-forms-and-changes 1.4.
I also have no insight, and have no clue how that even happened in the first place without me explicitly doing so. I reverted the guilty commit. Sorry about the disruption here.
Patched above! No worries.
While working on https://github.com/phetsims/arithmetic/issues/196 @Luisav1 and I were having issues with the level selection icons and layouts matching up. Upon inspecting the scenery helper we saw that an alignBox was pushing down our leftmost icon and causing other shifting behavior in the buttons.
We found that
LevelSelectionButtonGroup
was using analignGroup
to match the bounds of icons across all buttons. However, this was conflicting with the scaling logic inLevelSelectionButton
This scaling logic accurately downsized icons to fit within the button sizes provided in options, however when the bounds of the
alignBox
were scaled down, that triggered anupdateLayout
inAlignGroup
, that would then reset the bounds to match the largest height of the alignBoxes in the group. Since no other AlignBoxes had been visited yet the scaling was essentially reversed in this situation.