investigate how RectangularButtonView looks with 2 rounded corners

[pixelzoom]

This issue was extracted from #197.

Buttons with 2 rounded corners and 2 square corners are needed. For example, in sun.Carousel:

When support for this shape has been added to kite (see phetsims/kite#56), investigate how this shape works with RectangularButtonView, particularly with the pseudo-3D look, i.e.:

options.buttonAppearanceStrategy: RectangularButtonView.threeDAppearanceStrategy

[pixelzoom]

I did a quick test by changing line 90 of RectangularButtonView from this:

var button = new Rectangle( 0, 0, buttonWidth, buttonHeight, options.cornerRadius, options.cornerRadius, {
  fill: options.baseColor,
  lineWidth: options.lineWidth
} );

to this:

// Rectangle with rounded top corners and square bottom corners.
var button = new Path( new Shape()
  .moveTo( 0, buttonHeight )
  .lineTo( 0, options.cornerRadius )
  .arc( options.cornerRadius, options.cornerRadius, options.cornerRadius, Math.PI, 1.5 * Math.PI )
  .lineTo( buttonWidth - options.cornerRadius, 0 )
  .arc( buttonWidth - options.cornerRadius, options.cornerRadius, options.cornerRadius, 1.5 * Math.PI, 0 )
  .lineTo( buttonWidth, buttonHeight )
  .close(), {
  fill: options.baseColor,
  lineWidth: options.lineWidth
} );

Here's the "Buttons" screen from the sun demo. Both flat and 3D "looks" seem OK to me.

[pixelzoom]

Now the question is how to change the button API so that this button shape can be specified for all buttons that extend RectangularButtonView (i.e., RectangularPushButton, RadioButtonGroupMember, RectangularMomentaryButton, RectangularToggleButton, RectangularStickyToggleButton).

Assigning to @jbphet for his thoughts, since he authored much of this API.

[pixelzoom]

9/24/15 dev meeting:
Use the same options as proposed in phetsims/kite#56 (comment), that is:

topLeftCornerRadius:
topRightCornerRadius: 
bottomLeftCornerRadius:
bottomRightCornerRadius: 

Create a scenery.Rectangle if all radii are the same.
Otherwise create a scenery.Path with Shape.roundedRectangleWithRadii.

@jbphet will investigate.

[jbphet]

Implementing this depends upon phetsims/kite#56.

[jbphet]

I'm thinking that we should preserve cornerRadius as an option for backward compatibility and because it's a nice convenience. I'm planning to make the behavior as follows:

• default corner radius for all corners will be 4, as it is now
• setting any individual corner specifically will override the default for the corner but will not affect other corners
• a specific corner setting (e.g. topLeftCornerRadius : 10) will override a specific cornerRadius setting (e.g. cornerRadius : 0) in a given set of options.

I'll implement it this way initially but am willing to change it if other developers would like a different API behavior. The only thing that is a little weird about this is that Shape.roundedRectangleWithRadii defaults its corner radii to zero when unspecified, and buttons will default the corners to something other than zero when unspecified, but I think that is a minor inconsistency that we can live with.

[jbphet]

This is how the carousels now look:

...and here is some test code that was put into the ButtonView.js file in the sun demo but not committed:

    var buttonA = new RectangularPushButton( {
      content: new Text( '--- A ---', { font: BUTTON_FONT } ),
      cornerRadius: 10,
      listener: function() { message( 'Button A pressed ' + ( ++buttonAFireCount ) + 'x' ); }
    } );

    var buttonB = new RectangularPushButton( {
      content: new Text( '--- B ---', { font: BUTTON_FONT } ),
      listener: function() { message( 'Button B pressed' ); },
      leftTopCornerRadius: 0,
      rightTopCornerRadius: 20,
      baseColor: new Color( 250, 0, 0 )
    } );

    var buttonC = new RectangularPushButton( {
      content: new Text( '--- C ---', { font: BUTTON_FONT } ),
      listener: function() { message( 'Button C pressed' ); },
      leftTopCornerRadius: 0,
      rightTopCornerRadius: 5,
      leftBottomCornerRadius: 15,
      rightBottomCornerRadius: 20,
      baseColor: 'rgb( 204, 102, 204 )'
    } );

    var button1 = new RectangularPushButton( {
      content: new Text( '-- 1 --', { font: BUTTON_FONT } ),
      listener: function() { message( 'Button 1 pressed' ); },
      baseColor: 'rgb( 204, 102, 204 )',
      cornerRadius: 0,
      buttonAppearanceStrategy: RectangularButtonView.flatAppearanceStrategy
    } );

    var button2 = new RectangularPushButton( {
      content: new Text( '-- 2 --', { font: BUTTON_FONT } ),
      listener: function() { message( 'Button 2 pressed' ); },
      baseColor: '#A0D022',
      buttonAppearanceStrategy: RectangularButtonView.flatAppearanceStrategy,
      lineWidth: 1,
      cornerRadius: 0,
      leftBottomCornerRadius: 10,
      rightBottomCornerRadius: 10,
      stroke: '#202020'
    } );

...and this is what the buttons in the demo looked like with the options shown above (buttons 3 and 4 can be ignored):

[jbphet]

Assigning to @pixelzoom for review and discussion. One other specific question: Should we change the CarouselButton API to be a bit simpler and have something like a position option that can be either top, bottom, left, or right, and have this option control the corners as well as the arrow direction?

[pixelzoom]

@jbphet Re CarouselButton API... How about options.arrowDirection and options.cornerRadius, then let CarouselButton populate the proper '*CornerRadius' options to RectangularPushButton based on arrowDirection?

[jbphet]

I went ahead and implemented the suggestion in the previous comment, but it seems a little awkward to me. It would feel more intuitive IMO if we were to get rid of the arrowDirection option, add a position option with potential values of 'top', 'bottom', 'left', or 'right', and have both the arrow direction and the rounded corners derive from the value of the position option.

@pixelzoom - please let me know what you think of this suggestion. I'm happy to implement it, and can live with it as is if you have a strong preference.

[pixelzoom]

have both the arrow direction and the rounded corners derive from the value of the position option.

I'm fine with changing arrowDirection to position - same thing, different enumeration. But how can the cornerRadius possibly be derived from position (or arrowDirection)? The radius must be provided by the client. In the case of a Carousel client, the radius must match the Carousel's cornerRadius.

[pixelzoom]

Btw... If we think CarouselButton might be useful in situations other than Carousel, then I would feel more strongly about keeping the more general arrowDirection option. The position option only makes sense in relation to a container.

[jbphet]

Sorry, I guess I wasn't particularly clear. I'm thinking that the cornerRadius option would be retained, and the position option would be used to determine which corners were rounded, not the rounding radius (in addition to the direction of the arrow).

Since it is currently specific to the carousel (and is called CarouselButton), how about we make these changes, and generalize it if it ever becomes something more than a carousel button and the arrow direction and choice of rounded corners needs to be decoupled?

[pixelzoom]

I'm not clear on why position is less "awkward" (or better) than arrowDirection.

Here's the current option documentation in CarouselButton:

arrowDirection: 'up', // {string} direction that the arrow points, 'up'|'down'|'left'|'right'

This would presumably become:

position: 'top', // {string} where the button is located in the Carousel, 'top'|'bottom'|'left'|'right'

I don't like this because it implies knowledge about where the Carousel is going to put the buttons. And we've already determined that Carousel may offer different strategies for positioning next/previous buttons in the future. See the various layout examples shown in #166 (comment).

On the client side of the API, here's the current calls in Carousel:

    var nextButton = new CarouselButton( _.extend( { arrowDirection: isHorizontal ? 'right' : 'down' }, buttonOptions ) );
    var previousButton = new CarouselButton( _.extend( { arrowDirection: isHorizontal ? 'left' : 'up' }, buttonOptions ) );

Here's what the revised calls would look like:

    var nextButton = new CarouselButton( _.extend( { position: isHorizontal ? 'right' : 'bottom' }, buttonOptions ) );
    var previousButton = new CarouselButton( _.extend( { position: isHorizontal ? 'left' : 'top' }, buttonOptions ) );

In both cases, the Carousel orientation needs to be converted to a CarouselButton option. And I don't see how position is any better than arrowDirection here.

So my 2 cents would be to leave this as is.

[jbphet]

Okay, let's leave it as is. Closing.