Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

clean up UnderDevelopmentPlane ? #69

Closed
pixelzoom opened this issue Jul 19, 2017 · 5 comments
Closed

clean up UnderDevelopmentPlane ? #69

pixelzoom opened this issue Jul 19, 2017 · 5 comments
Assignees

Comments

@pixelzoom
Copy link
Contributor

In #30 (comment), @zepumph said:

The UnderDevelopmentPlane.js is a bit messy. I don't think it is a big deal because it is "temporary" (what ever that means by phet sim development standards). It could use some variable names and less nesting. @pixelzoom it's your call if you think that is worth your time for such a temporary and simple Node.

What do you find messy about it? There's more "nesting" of instantiation than I typically use, but certainly not any more than some developers. The only duplication that I see is PhetFont(16), and that's just coincidental - the text and link could (and did) have different font sizes.

@zepumph
Copy link
Member

zepumph commented Jul 19, 2017

My thoughts were about

    var vBox = new VBox( {
      align: 'left',
      spacing: 20,
      children: [
        new Text( underDevelopmentLine1String, {
          font: new PhetFont( 22 ),
          maxWidth: maxTextWidth
        } ),
        new VBox( {
          align: 'left',
          children: [
            new Text( underDevelopmentLine2String, {
              font: new PhetFont( 16 ),
              maxWidth: maxTextWidth
            } ),
            new RichText( linkText, {
              links: true, // allow links in linkText
              font: new PhetFont( 16 ),
              maxWidth: maxTextWidth
            } )
          ]
        } )
      ]
    } );

I too don't think it is so bad, I mentioned it because it was atypical more than it being really problematic.

@pixelzoom
Copy link
Contributor Author

What don't you like about it? The nested structure?

@ariel-phet
Copy link

Specificity @zepumph ....Specificity :)

@zepumph
Copy link
Member

zepumph commented Jul 19, 2017

Sorry folks. Yes indeed the nested structure was what I was looking at. After discussing it through, I think it is best as is. I mentioned it because I was not used to so much node creation without variable names. To me it looked sloppy. I feel good to close now without action.

@pixelzoom
Copy link
Contributor Author

I agree, nesting like this is not desirable - difficult to read and maintain. But plan to see it in other PhET code, because it's a style preferred by some of the other PhET developers.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants