phetsims / kite

A library for creating, manipulating and displaying 2D shapes in JavaScript.
MIT License
13 stars 6 forks source link

rectangle with 2 rounded corners #56

Open pixelzoom opened 9 years ago

pixelzoom commented 9 years ago

From https://github.com/phetsims/sun/issues/197 ...

Add support (to Shape?) for creating a Shape that is a rectangle with 2 of its adjacent corners rounded, and the other 2 corners squared.

jonathanolson commented 9 years ago

As a more general pattern, can we supply a cornerRadius for each corner, OR boolean flags for which corners are rounded, and which ones aren't?

I'd hate to have to handle "rectangle with 1 or 3 rounded corners" separately.

Thoughts?

samreid commented 9 years ago

What if someone asks for a partially rounded triangle?

pixelzoom commented 9 years ago

What if someone asks for a partially rounded triangle?

Then they have a blank slate to write their own, because there is no scenery.Triangle, and I don't see any triangle functions in kite.Shape.

pixelzoom commented 9 years ago

can we supply a cornerRadius for each corner,

Since the most common use case is to have the same radius for all rounded corners, this duplicates the problem that we have with scenery.Rectangle (https://github.com/phetsims/scenery/issues/467).

OR boolean flags for which corners are rounded, and which ones aren't?

OK with me.

jonathanolson commented 9 years ago

Any preferences between reusing Shape.roundedRectangle or a new function, and also between optional arguments for corners:

function( x, y, width, height, arcw, arch, isUpperLeftSharp, isUpperRightSharp, isLowerRightSharp, isLowerLeftSharp )

or more of an options pattern:

function( x, y, width, height, { cornerRadius: ..., roundUpperLeft: false, ... } )

or any other combination?

jonathanolson commented 9 years ago

Marked for developer-meeting as an API question.

pixelzoom commented 9 years ago

9/24/15 dev meeting:

// function signature
Shape.roundedRectangleWithRadii = function( x, y, width, height, {
topLeftCornerRadius:
topRightCornerRadius: 
bottomLeftCornerRadius:
bottomRightCornerRadius: 
} ) { ... };

// use case
var cornerRadius = 20;
var rect = Shape.roundedRectangleWithRadii( 0, 0, 200, 100, {
topLeftCornerRadius: cornerRadius,
topRightCornerRadius: cornerRadius
} );
jonathanolson commented 9 years ago

Completed, assigning for review, and to consider not including "CornerRadius" in every corner option. Any opinion on:

// use case
var cornerRadius = 20;
var rect = Shape.roundedRectangleWithRadii( 0, 0, 200, 100, {
  topLeft: cornerRadius,
  topRight: cornerRadius
} );
pixelzoom commented 9 years ago

I guess the only reason to include "CornerRadius" in each option name would be if we think there will additional options. If we don't think there will be additional options, then I recommend renaming the options parameter to cornerRadii. Any other opinions?...

pixelzoom commented 9 years ago

Since Shape.roundedRectangleWithRadii is unlikely to be used for other types of rectangles, and is unlikely to have non-radii options, I think it's appropriate to change its signature to:

Shape.roundedRectangleWithRadii( x, y, width, height, cornerRadii )

where cornerRadii defaults to:

{
  topLeft: 0,
  topRight: 0,
  bottomLeft: 0,
  bottomRight: 0
}
samreid commented 9 years ago

The suggestion in https://github.com/phetsims/kite/issues/56#issuecomment-143797019 seems good to me.

jbphet commented 9 years ago

+1 for https://github.com/phetsims/kite/issues/56#issuecomment-143797019.

jonathanolson commented 9 years ago

Changed, assigning @pixelzoom for review.

pixelzoom commented 9 years ago

:+1: Closing.

jbphet commented 9 years ago

Reopening - I believe the convention in other PhET libraries is to specify 2D options with the x value first and the y value second, e.g. leftTop instead of topLeft (see Node.js in Scenery). Shouldn't we follow the same convention here?

pixelzoom commented 9 years ago

OK with me. Back to @jonathanolson.

samreid commented 9 years ago

Yes, same convention please.