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

rectangle with 2 rounded corners #56

Open
pixelzoom opened this issue Sep 17, 2015 · 17 comments
Open

rectangle with 2 rounded corners #56

pixelzoom opened this issue Sep 17, 2015 · 17 comments

Comments

@pixelzoom
Copy link
Contributor

From phetsims/sun#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
Copy link
Contributor

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
Copy link
Member

samreid commented Sep 22, 2015

What if someone asks for a partially rounded triangle?

@pixelzoom
Copy link
Contributor Author

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
Copy link
Contributor Author

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 (phetsims/scenery#467).

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

OK with me.

@jonathanolson
Copy link
Contributor

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
Copy link
Contributor

Marked for developer-meeting as an API question.

@pixelzoom
Copy link
Contributor Author

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
Copy link
Contributor

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
Copy link
Contributor Author

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 pixelzoom assigned jonathanolson and unassigned pixelzoom Sep 28, 2015
@pixelzoom
Copy link
Contributor Author

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
Copy link
Member

samreid commented Sep 28, 2015

The suggestion in #56 (comment) seems good to me.

@jbphet
Copy link
Contributor

jbphet commented Sep 28, 2015

+1 for #56 (comment).

@jonathanolson
Copy link
Contributor

Changed, assigning @pixelzoom for review.

@pixelzoom
Copy link
Contributor Author

👍 Closing.

@jbphet
Copy link
Contributor

jbphet commented Oct 1, 2015

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?

@jbphet jbphet reopened this Oct 1, 2015
@pixelzoom
Copy link
Contributor Author

OK with me. Back to @jonathanolson.

@pixelzoom pixelzoom assigned jonathanolson and unassigned pixelzoom Oct 1, 2015
@samreid
Copy link
Member

samreid commented Oct 2, 2015

Yes, same convention please.

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

5 participants