Review Scenery Layout documentation

[jonathanolson]

See https://phetsims.github.io/scenery/doc/layout

I'll hopefully have the documentation browsable more easily soon (and at a public URL).

• [x] @AgustinVallejo
• [x] @pixelzoom
• [x] @jbphet
• [x] @jessegreenberg
• [x] @zepumph
• [x] @marlitas
• [x] @samreid

Please check yourself off and unassign once you've reviewed it!

[pixelzoom]

In Slack#developer, @jonathanolson said:

Moved scenery's github pages branch to master, and added a dist/ directory for built persistent versions, so https://phetsims.github.io/scenery/doc/layout should update automatically on commits to master

[pixelzoom]

I've completed my review of the Scenery Layout documentation.

The FlowBox and GridBox sections are very nice. After reading, I feel like I have a good general understanding of them, and could get to work with them immediately.

The Sizeables section is a little abstract (necessarily, I guess). But I think I understand what's going on here, and how it applies to FlowBox et. al.

The Constraints sections are too sparse, not an effective overview. I was left with so many general questions: When do I use Constraints vs Flowbox/Gridbox? Can I use them together? When do I use ManualConstraint vs FlowConstraint vs GridConstraint? Will I need to use them often, or will FlowBox et.al. generally be sufficient? etc. If it's important that developers understand Constraints, then I recommend putting more work into this section.

Other specific things that need attention are in the checklist below.

• [x] It's currently impossible to copy text from the documentation. That makes it difficult to copy-paste examples, or copy-paste lines of text into GitHub issues (like this one) for review comment. Can that please be changed asap?

• [x] A table of contents at the top, with links to (at least) the main sections, would be helpful.

• [x] The first sentence of the documentation includes a link to a "number of examples". That link works OK in a locally-built copy of scenery. But when viewing the documentation from https://phetsims.github.io/scenery/doc/layout, it fails with a 404 error because the link resolves to https://phetsims.github.io/sun/sun_en.html?brand=phet&ea&debugger&screens=4. Relevant code in layout.html:

  <p>
    If you have the main PhET libraries checked out (sun, etc.), there are a
    <a href="../../sun/sun_en.html?brand=phet&ea&debugger&screens=4">number of examples</a> that show layout in action with
    common components.
  </p>

• [x] The code for the examples in LayoutScreenView.ts is almost entirely undocumented. At the least, each "demo" function should have a comment that summarizes what it's demonstrating, and notes anything important that you want the reader to notice or take away from the example.

• [x] Referring to primary and secondary orientation is confusing, because the options are orientation and align. Consider having primary and secondary axes instead. Orientation is done on the primary axis, alignment is done on the secondary axis. (I don't feel too strongly about this, so by all means ask other devs if there's a confusion factor here.) Relevant paragraph, but also elsewhere in the doc (and probably the code?):

      NOTE: For FlowBox, the orientation provided is typically called the <em>primary</em> orientation. This is the
      orientation along which each Node in a line is positioned. The <em>secondary</em> orientation is the opposite one,
      along which each Node is aligned. e.g. for an HBox, its primary orientation is horizontal, and its secondary
      orientation is vertical (so its elements will be positioned with increasing x values, and its align will control
      the y values).

• [x] The current state of LayoutBox seems problematic. The doc says:

For cases where the orientation needs to be determined programmatically, use FlowBox:

There are many places that use LayoutBox, assuming that they are the base class of HBox and VBox. That is no longer the case. Should LayoutBox be converted to extend FlowBox? Or should uses of LayoutBox be eagerly replaced with FlowBox? Or at the very least, should LayoutBox be deprecated?

• [x] When I got to the Resizing example, the entire document below this example shifts up and down as the example resizes. This is making the documentation very difficult/distracting for me to read. Can the example be put in a div that's large enough to accommocate it's max height? In the meantime, I've added resize: false to that example in my working copy of layout.html.

• [x] In the first grow example for FlowBox, it would be useful to note that the value of grow is a proportion.

• [x] In the grow examples for GridBox, it would be useful to point out that the first example uses grow (which applies to both x & y dimensions) while the second example uses xGrow and yGrow.

• [x] In the width/height section for GridBox:

Cells can take up more than one row/column with the width/height layout options:

If I understand correctly, width and height are proportional in this context. That seems problematic/error-prone, because width and height are absolute throughout scenery. Consider a different name for these options. And in any case, note here that they are proportional.

• [x] Use code-font styling for things like resize: false, grow, layoutOptions, etc. This is an issue throughout the doc. For example:

Resizing/layout can be disabled with resize:false:

Nodes with preferred sizes can be added, and the grow in layoutOptions will attempt to put extra space into that cell

• [x] Fix places where you tried to use Markdown formatting for code, for example:

`preferredWidth` / `localPreferredWidth`). These separate coordinate frame versions will be kept in sync (and are

• [x] typos:

-    <p>Dividers are also available for easy of use (dividers at the visible start/end and duplicates will be marked as invisible, while all other dividers will be marked as visible:</p>
+    <p>Dividers are also available for easy of use (dividers at the visible start/end and duplicates will be marked as invisible, while all other dividers will be marked as visible):</p>

• [x] grammar:

- <p>Justification controls how extra space is allocated around cells (after any growing possible has been done):</p>
+ <p>Justification controls how extra space is allocated around cells (after any possible growing has been done):</p>

- <p>Similarly, alignment can also be customized to individual cells:</p>
+ <p>Similarly, alignment can also be customized by individual cells:</p>

• [x] consistency:

- <p>Spacing between lines (rows/cols) can also be added, which applies when wrapped:</p>
+ <p>Spacing between lines (rows/columns) can also be added, which applies when wrapped:</p>

[pixelzoom]

Discussed this one with @jonathanolson:

• [x] The current state of LayoutBox seems problematic. The doc says:

  For cases where the orientation needs to be determined programmatically, use FlowBox:

There are many places that use LayoutBox, assuming that they are the base class of HBox and VBox. That is no longer the case. Should LayoutBox be converted to extend FlowBox? Or should uses of LayoutBox be eagerly replaced with FlowBox? Or at the very least, should LayoutBox be deprecated?

I deprecated LayoutBox in the above commit:

/**
 * @deprecated use FlowBox
 */
export default class LayoutBox extends Node {

And I'm going to replace LayoutBox with HBox or VBox pro-actively, whereever possible in my code.

[AgustinVallejo]

Got to read most of it! The first examples are very useful and surely I'll be coming back to this documentation to use them in my own code. As @pixelzoom said, FlexBox and GridBox are nicely written, but after that, it gets a little bit complicated, at least for begginers. After *Sizable I got a little bit lost, as I'm not used to some of the terminology used in there.

Here are my observations:

• [x] Typo just before the VBox example:

• For vertical line-based layout, use HBox:

• For vertical line-based layout, use VBox:

• [x] It seems to me that after the resize section, all the page becomes jiggly! It becomes difficult to read. Maybe enclose the resizing FlowBox in a fixed height div? I understand the page shouldn't move when you scroll past that section but it isn't working as intended.

• [x] In the justify section, shouldn't the first box move to the left? It says justify:left but has the same justification as justify:center.

• [x] The LineSpacing example allows the FlowBox to be compressed to a very small square, I'm not sure if that's what wrapping means or if it's unintentional.

• [x] I think the stretch example works best contextually if it were next to grow.

• [x] The RemoveRow/RemoveColumn example doesn't seem very clear. The functions are easy to understand, but I don't get the example with the color boxes, is it trying to show the absence of some colored rows?? Maybe with numbers (1,,,4,5,6) it would be clearer?

[marlitas]

Have already implemented this in Mean: Share and Balance. The documentation was overall very clear and easy to follow! A couple of notes to take or disregard:

• [x] In its default size, the example in the justify section does not demonstrate what I would expect the justifications to look like. When skimming documentation I personally use the examples as an initial guide of "will this tool do what I'm looking for?". I also did not realize the divs could be resized until a second/third look at the documentation. I would suggest having the default size of the justification example be slightly larger so that its purpose is clearer from the beginning.

• [x] Alternatively making it clearer which divs can be adjusted, since the bottom right handle is somewhat hidden when glancing through.

• [x] widthSizable is set twice in this example (20 and 25) in the GridBox sizable section.

  

• [ ] When reading through the Sizables section I didn't quite understand why there's a new concept that is so similar to stretch. It seems to do the same thing as stretch, but additionally provides limiting a stretch to the x or y axis. Perhaps these can be consolidated?

• [x] layoutOptions suggests that it is preferred to use mutateLayoutOptions specifically because of GridBox. Is this a concern only with GridBox then, and mutateLayoutOptions vs node.layoutOptions = is irrelevant in FlowBox? Or is there another overarching reason to why this should be the default in general?

• [x] layoutOrigin was a bit confusing to follow. It states that "the Entire FlowBox will be at the origin of the first line." The example then assumes that "first line" equates to first line of only text. In the code example:

  

• A new circle is being created and then we are adding children bigText and normalText. Are these scenery text objects? If we just input a string as a child will it behave the same way? What is the data type limitation on what the FlowBox will consider a "first line"?

• [x] I agree with @pixelzoom that ManualConstraints, FlowConstraints, and GridConstraints was difficult to follow. Seems like they could be useful tools, but I have no clue how to properly implement or when implementing would be appropriate.

[samreid]

Nice work! Some of my comments are more brainstorm-y and impractical. Still, I wanted to jot all my thoughts down. Feel free to veto or move to side issues.

• [x] The HBox has a fit minimum width but the VBox can shrink even lower. Is that by design?

  

• [x] Can we design/add a favicon? It currently looks like this:

  

• [ ] The page gives several warnings in the console:

  

• [x] Should there be a "playground" where you can edit the code and see how it updates?

• [x] Prefixing all types with scenery. like scenery.Circle is not idiomatic for our project. Is that how we want clients to use it?

• [x] What happens if you use a VDivider in an HBox?

• [x] Can we double check that we prefer x and y to row and col?

• [x] I tested removing the return and scenery. from these arrow functions and it seemed a little clearer:

  const box = new scenery.GridBox( {
    rows: _.range( 0, 8 ).map( row => {
      return _.range( 0, 8 ).map( column => {
        return new scenery.Rectangle( 0, 0, 20, 20, {
          fill: new scenery.Color( row * 32, column * 32, 1 )
        } );
      } );
    } )
  } );

  and it looked like this:

const box = new GridBox( {
  rows: _.range( 0, 8 ).map(
    row => _.range( 0, 8 ).map(
      column => new Rectangle( 0, 0, 20, 20, {
        fill: new Color( row * 32, column * 32, 1 )
      } ) ) )
} );

• [x] It's a very long page without any index. I'm wondering if we would prefer a design more like a single page web app (but it may be less searchable). Imagine a playground with an editable text area:

• [x] For debugging, can we display the grid for gridbox, like how we ?showPointerAreas?
• [x] I would love a section that shows an exemplar sim with real-world layout issues, and arrows/schematics/overlays that show which layouts are appropriate where.

[samreid]

We touched base on this today. @pixelzoom recommended a new issue https://github.com/phetsims/scenery/issues/1422. But first, @jonathanolson will address this issue and #1419.

@jonathanolson will address feedback so far before other devs review.

[jonathanolson]

Can we double check that we prefer x and y to row and col?

Added a note to dev meeting.

[pixelzoom]

Helper.ts and LayoutScreenView.ts were missed in the replacement of x/y to column/row. I'm not sure why this was showing up in CT, but tsc was failing in my working copy after a pull-all.sh. It was blocking other work I'm doing, so I fixed them in the above commits. @jonathanolson please review.

[jonathanolson]

For debugging, can we display the grid for gridbox, like how we ?showPointerAreas?

I have more improvements to do with this, but I extended the Helper to show areas/boxes for layout containers once they are "selected" (if you enable it for every layout container, it's very cluttered, so a query parameter didn't work as well):

It shows spacing, empty areas and margins for GridBox/FlowBox, and I'll be able to add more specific info in the future.

It also adds the ability for arbitrary nodes to add a getHelperNode() function that will show similar content in the helper (possibly useful for sim-specific or common code debugging).

[pixelzoom]

Several months ago, we decided that @jonathanolson would update the documentation, based on the feedback that's been given so far. Then the rest of the dev team would review the revised documentation and provide further feedback. Despite the fact that this has been high priority, it doesn't look like any of that has moved forward.

Assigning to @kathy-phet to prioritize.

[marlitas]

If I understand correctly, width and height are proportional in this context. That seems problematic/error-prone, because width and height are absolute throughout scenery. Consider a different name for these options.

@pixelzoom I looked through CSS Grid Layout documentation as a basis for a better name for height/width and came across span. I do believe that @jonathanolson's implementation of spanning columns or rows is much more straightforward than what CSS provides, but perhaps renaming height to verticalSpan and width to horizontalSpan might provide more clarity on the functionality of those properties. Thoughts?

[samreid]

@marlitas and I are triaging the "subgroup" and we believe this issue does not need to be listed in the subgroup column. @marlitas and @jonathanolson have another project board that lists scenery layout issues, and this is featured prominently there. Moving to "done discussing" for now.

[jonathanolson]

When reading through the Sizables section I didn't quite understand why there's a new concept that is so similar to stretch. It seems to do the same thing as stretch, but additionally provides limiting a stretch to the x or y axis. Perhaps these can be consolidated?

@marlitas I'm not sure I understand, which concept is similar to stretch? If it is the sizable, does the above commit resolve this?

[jonathanolson]

The page gives several warnings in the console:

I think it is not horrible if there are some warnings in the console. @samreid is that acceptable? (upgrading/migrating to newer bootstrap/highlightjs/codemirror might fix this, but would be a large hassle)

[jonathanolson]

Responded to / fixed all of the noted improvements.

@kathy-phet, should @chrisklus, @jbphet, @jessegreenberg, and @zepumph review this?

Unassigning.

[samreid]

I think it is not horrible if there are some warnings in the console. @samreid is that acceptable? (upgrading/migrating to newer bootstrap/highlightjs/codemirror might fix this, but would be a large hassle)

Yes, as long as the functionality is correct it seems OK for this to show in the console in this case.

[marlitas]

@marlitas I'm not sure I understand, which concept is similar to stretch? If it is the sizable, does the above commit resolve this?

I'm not exactly sure to what I was referring. I don't think the Note: you added is necessary. It is very possible that the Marla who made that review comment was really not understanding that Sizable is a mixin. Perhaps it would be helpful to really drive home that Sizable is the foundation for a lot of dynamic layout? That might be the thing that feels like is missing from that piece of documentation. But honestly I think it works either way, so whichever you prefer.

[jonathanolson]

Perhaps it would be helpful to really drive home that Sizable is the foundation for a lot of dynamic layout?

Suggestions for the details of this? It seems like for highly-nested layouts, it is the core, but a lot of layout is done without nesting.

[marlitas]

Suggestions for the details of this? It seems like for highly-nested layouts, it is the core, but a lot of layout is done without nesting.

Mmmm I see what you're saying, like LayoutConstraints and such don't always need Sizable but are a part of dynamic layout. I think these sentences could maybe be adjusted to reflect that Sizable is a key part of their dynamic behavior as bounds change.

"When the preferred size is set, the Node should adjust its own layout so that it takes up that size. This includes FlowBox (VBox/HBox), GridBox, Panel, and a growing list of Nodes."

Perhaps something like: "FlowBox (VBox/HBox), GridBox, Panel, and a growing list of Nodes rely on Sizable to update their layout as the bounds of their content changes. When the preferred size is set, the Node should adjust its own layout so that it takes up that size."

I think front loading the explanation with recognizable components we've already talked about in the documentation helps connect the relationship. Being the last sentence in that first paragraph it feels a bit more like an after thought.

[jonathanolson]

How is the above wording? (would need to check out layout-2024 branch for scenery/sun to see if in the docs).

[marlitas]

@jonathanolson I think that's great. Much clearer. Thanks for making those changes!

In regards to getting some other developer review on the documentation I'll bring that up in planning today. I'm going to go ahead and leave this assigned to me, and unassign from @kathy-phet. I also will update the list to remove @chrisklus since he is now focused on website.

[jessegreenberg]

I have some review notes now since I was working with this document this iteration.

• I am having a hard time understanding verticalSpan/horizontalSpan, can you describe a bit more about how it should behave? I tried to play with the values in the sandbox but don't understand the behavior I am seeing. Is there a way to demonstrate it in a more simple example that uses fewer options?

• The example for GridBox stretch is the same as the example for GridBox sizable. Is that a mistake? Wondering if widthSizable/heightSizable should be somewhere in the sizable example.

• Can you describe the difference between margin and spacing in the documentation? EDIT: It is there.

• I think it would be helpful to document the behavior of some of the justify values. For example, half spacing at the ends for spaceAround and difference between spaceBetween was not obvious to me.

• There is a minor bug where resizing a demo selects text. Would something like this be reasonable to add? Probably bad for multi-touch. I tried preventDefault() but couldn't use it because of passive events.

         new DragListener( {
           start: () => {
             document.body.style.userSelect = 'none';
           },
           end: () => {
             document.body.style.userSelect = '';
           }

• I have found a bug where sometimes the sandbox doesn't clear while a new instance loads on top of it. Have not been able to reproduce consistently but will try.

• I noticed a bug with the handles where the container has some jitter. Ill try to get more details and debug a bit more.

Otherwise, this document has been super helpful for me and these are great examples!

EDIT:

I noticed a bug with the handles where the container has some jitter. Ill try to get more details and debug a bit more.

This is specifically in the last AlignBox example for the horizontal drag handle. I looked around for a while but couldn't find anything. It may be about performance but it feels more like "two containers trying to adjust layout at the same time" kind of issue. Its also easy to see in the "wrap" demo.

EDIT:

I have found a bug where sometimes the sandbox doesn't clear while a new instance loads on top of it.

I looked into this but couldn't find a pattern. And it very rarely happens. Maybe there is a type of error that it can't recover from?

[jbphet]

I just did a light review, but didn't read through it in great detail since it seems like others have already done so. My biggest suggestion would be that it should have an "Introduction" section that describes what is and isn't covered, and how to use the document. Right now it begins with the phrase, "If you're using a built version of Scenery, you'll need to add the relevant namespaces to objects" which seems like an oddly specific way to start things off. This should probably be in some section like "Using a Built Version of Scenery" somewhere else.

It would also be worthwhile to mention that searching through the PhET code base is also a way to see examples of how the various layouts can and should be used.

That's all I have, at least for now.

[jbphet]

I'm assigning to @zepumph, since he's the last person on the original list of reviewers, and to @jonathanolson so that he can respond to my comment above.

[zepumph]

I'm excited and happy to take a look soon. Let's have @jonathanolson handle the above items and then I'll take a pass at things.

[jonathanolson]

@jessegreenberg I believe I've handled your comments above, can you review?

[jonathanolson]

@jbphet, can you check out the new intro section added above?

[jessegreenberg]

Yep, this is very helpful. Changes look great, thanks @jonathanolson!

[jbphet]

Intro looks great, thanks for adding it.

[jbphet]

Assigning @zepumph based on his comment above.

[zepumph]

review:

• [x] scenery/doc/index.html does not mention the word "layout", nor does http://localhost:8080/scenery/doc/a-tour-of-scenery.html.
• [x] minor layout issue with hyperlinks on the left:
• [x] layout examples have the following assertion: Error: Assertion failed: phet.chipper.localeData global expected. As a result, I did not review that file, let me know if you would like me to.
• [x] I think it may flow better if the items for Sizables and layoutOptions went first, above FlowBox and GridBox. They provide some nice generalities. Not sure if layoutOrigin should come with or stay closer to the bottom. Up to you.
• [x] In the same spirit, I believe that AlignBox may want to go above Constraint, since it is still more in the "layout containers" section.
• [x] I think that you should add an example to AlignBox using AlignGroup, otherwise the doc doesn't really help me get to a point where I could implement it in practice. Not sure if it is worth a whole section on AlignGroup, up to you.
• [x] Noting that in layout-examples, Reflection and Rotation sections aren't really at a spot where I would close this issue.

Overall good job. Things are clear, and I learned a lot. Thanks for all the hard work.

[zepumph]

• [x] We should run layout exemplars as a CT page load test

[jonathanolson]

scenery/doc/index.html does not mention the word "layout", nor does http://localhost:8080/scenery/doc/a-tour-of-scenery.html.

I mentioned it above in the index docs (and put in links to exemplars). However I'm not sure a direct layout link from the tutorial is appropriate. Scenery docs in this format are woefully incomplete, we'll want to restructure to better documentation pages in the future.

[jonathanolson]

We should run layout exemplars as a CT page load test

It is the one phet-lib test right now! It uses a built form of phet-lib though, so once we update phet-lib we'll need to patch it up (and figure out a stub for localeData).

[jonathanolson]

Noting that in layout-examples, Reflection and Rotation sections aren't really at a spot where I would close this issue.

Can you check https://phetsims.github.io/phet-lib/doc/layout-exemplars? (I think it is more fleshed out than the copy you saw in scenery-phet). Additionally, I think it is handled by the issue in https://github.com/phetsims/scenery/issues/1474 as a separate item.

@zepumph can you review the changes above to see if those look good to you? If all looks good, I think we can close this.

[zepumph]

This is all excellent. Thanks!

[jonathanolson]

Thanks, closing!