generalize GridBox
Closed this issue · 24 comments
GridLayoutBox (used for layout of the Components radio buttons) looks nice, and I think it will be a candidate for migrating to scenery-phet or scenery. So I'm going to spend a little time on cleanup and generalization.
Generalization has been done. Renamed to GridBox.
Next step is either:
(1) Stop here and let the next developer who needs GridBox move it to a common-code repo. This would be typical PhET procedure.
(2) Decide which common-code repo it belongs in, move it now, code review, etc. This might be better if we want developers to know that it exists.
Labeling for developer meeting to decide. Developers, please have a look at GridBox.js.
@ariel-phet if you'd like to make a decision, feel free to preempt developer discussion.
Does the GridBox assume its children are all the same size? To explore this question, I started with scale 0.2 and bumped it up by 0.2 for each button, and the result looked like this:
Is this as expected?
Index: js/common/view/ComponentStyleRadioButtonGroup.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- js/common/view/ComponentStyleRadioButtonGroup.js (revision 99d6aa221dddd461541238aa4d4881ad18db1078)
+++ js/common/view/ComponentStyleRadioButtonGroup.js (date 1569470322668)
@@ -22,6 +22,8 @@
const VectorAdditionConstants = require( 'VECTOR_ADDITION/common/VectorAdditionConstants' );
const VectorAdditionIconFactory = require( 'VECTOR_ADDITION/common/view/VectorAdditionIconFactory' );
+ let scale = 0.2;
+
class ComponentStyleRadioButtonGroup extends Node {
/**
@@ -35,8 +37,9 @@
// Create the radio buttons
const buttons = [];
ComponentStyles.VALUES.forEach( componentStyle => {
+ scale = scale + 0.2;
buttons.push( new RadioButtonGroupMember( componentStyleProperty, componentStyle,
- _.extend( {}, VectorAdditionConstants.RADIO_BUTTON_GROUP_OPTIONS, {
+ _.extend( { scale: scale }, VectorAdditionConstants.RADIO_BUTTON_GROUP_OPTIONS, {
content: VectorAdditionIconFactory.createComponentStyleRadioButtonIcon( componentStyle )
} ) ) );
} );
I'm curious what @jonathanolson did in area model, because I thought there was this sort of component there. Does the generalization that @pixelzoom worked on here support your use case?
@samreid asked:
Does the GridBox assume its children are all the same size?
Apparently it does. That would need to be fixed, presumably with an AlignGroup.
And I'm guessing that it made that assumption because it was an inner type of a radio button group, where all buttons are the same size.
In the commits above, AlignGroup and AlignBox are used to ensure that each cell in the grid is the same size, and that nodes are aligned in those cells based on options xAlign and yAlign.
Here are some examples, using @samreid's patch from #190 (comment) that scales each of the radio buttons differently.
{ xAlign: 'center', yAlign: 'center' } (the defaults):
{ xAlign: 'left', yAlign: 'top' }:
{ xAlign: 'right', yAlign: 'bottom' }:
Here's an example with option columns: 3, and no scaling of the radio buttons. Children are processed in row-major order, so cells are filled left-to-right, top-to-bottom.
In #190 (comment), @zepumph said:
I'm curious what @jonathanolson did in area model, because I thought there was this sort of component there. Does the generalization that @pixelzoom worked on here support your use case?
I'd also like to hear from @jonathanolson.
Does the generalization that @pixelzoom worked on here support your use case?
This looks useful, and could probably be used in GenericLayoutSelectionNode. I had the simple case where every icon was the same size, so it could just use HBoxes in a VBox.
@jonathanolson would you recommend moving this to scenery or scenery-phet?
I think scenery would be the best option (as we already have pure-layout there).
One thing that's helpful about our current layout containers is that you can change their children and they re-layout. Would that be an option for GridBox?
Changing children could be an option, but it's not currently supported. And it (probably?) wouldn't be the same API as LayoutBox/HBox/VBox, since it's not a subclass of LayoutBox. Suggestions for the API?
In an ideal world, I'd imagine it would function similarly to LayoutBox, but with the differences based on it being a grid.
GridBox has 1 child, a VBox that handles layout of the rows (HBox). So if we want to support the same "dynamic children" API as LayoutBox, we'll need to do some "clever" things to support addChild, removeChild, getChildren, etc. -- essentially to make the children of GridBox look different than reality.
So I'm leaning towards a different API. Something like:
/**
* @param {Node[]} contents - the contents of the grid, in row-major order
* @param {Object} [options]
*/
constructor( contents, options )
/**
* Gets the contents of the grid, in row-major order.
* @returns {Node[]}
* @public
*/
getContents()
/**
* Sets the contents of the grid, in row-major order.
* @param {Node[]} content - the contents of the grid, in row-major order
* @public
*/
setContents( contents )
Very true, those constraints change things. That sounds like a good API to me.
But GridBox would still have addChild/removeChild etc. Would we just ask that clients not call those?
But GridBox would still have addChild/removeChild etc. Would we just ask that clients not call those?
Presumably. An alternative would be to reimplement it fully so it doesn't rely on the child containers.
An alternative would be to reimplement it fully so it doesn't rely on the child containers.
I know it was quick and easy to make an initial implementation based on a VBox of HBoxes, but it seems a good idea to investigate implementing the layout without needing child containers.
But GridBox would still have addChild/removeChild etc. Would we just ask that clients not call those?
There would be no harm in using addChild/removeChild. But they would decorate the GridBox, rather than change its contents.
I know it was quick and easy to make an initial implementation based on a VBox of HBoxes, but it seems a good idea to investigate implementing the layout without needing child containers.
That's not something I'm interested in. If that's the consensus, then we'll leave GridBox in vector-addition, and the next developer who needs a grid layout can decide how to proceed.
There would be no harm in using addChild/removeChild. But they would decorate the GridBox, rather than change its contents.
We may decide that is acceptable behavior, but it would be different than the behavior in VBox or HBox (which organizes elements added with addChild).
I've implemented the API described in #190 (comment). It took ~10 minutes, and the implementation is straightforward.
If someone wants to pursue a more complicated implementation, go for. But this seems like a very appropriate application of HBox and VBox. And I don't feel that matching the LayoutBox API is worth the added complexity and costs.
I think I mostly agree with @pixelzoom, but I'm also curious about a "half" solution where we factor out a way to change the Node list, and call some sort of "updateView" function which recreates the H/Vboxes. Would this cover all of the dynamic cases we come across?
I'm not sure how the preceding comment could work. If you call addChild, it cannot simply update itself with all of this.children because they are doubly-nested in the containers. At a minimum, we would need steps to gather children from the hbox and vboxes, and I'm not sure that's desirable.
Dev meeting 10/10/19 consensus:
Lots of questions and disagreement on what the API for GridBox should be. So this is going to remain in vector-addition, and the next developer who needs a grid layout can use this as inspiration (or not) for something that everyone is comfortable with putting in scenery.
Closing.