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).

• @AgustinVallejo
• @pixelzoom
• @jbphet
• @jessegreenberg
• @zepumph
• @marlitas
• @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.

• 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?

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

• 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>

• 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.

• 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).

• 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?

• 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.

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

• 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.

• 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.

• 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

• 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

• 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>

• 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>

• 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:

• 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.

[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:

• scenery/doc/index.html does not mention the word "layout", nor does http://localhost:8080/scenery/doc/a-tour-of-scenery.html.
• minor layout issue with hyperlinks on the left:
  
• 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.
• 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.
• In the same spirit, I believe that AlignBox may want to go above Constraint, since it is still more in the "layout containers" section.
• 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.
• 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]

• 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 #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!