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

Added roundRect support #41

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Added roundRect support #41

wants to merge 1 commit into from

Conversation

fgerodez
Copy link

Hi,

This is a pull request that exposes the new roundRect method.

As it is not currently supported on firefox I was wondering if we should add a warning in the usage section of the readme to link to a polyfill such as this one

The radii of the corners can be specified in much the same way as the CSS border-radius property.
[MDN docs](https://developer.mozilla.org/en-US/docs/Web/API/CanvasRenderingContext2D/roundRect)
-}
roundRect : Float -> Float -> Float -> Float -> List Float -> Command

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could it be considered to use the type system for specifying the points? We lose the CSS flexibility in this case, but gain some more clarity, I think. For example: type alias Radii = { topLeft : Float, topRight : Float, bottomLeft : Float, bottomRight : Float }.

Optionally, some helpers could be provided, like : allCorners : Float -> Radii.

Or, make Radii a sum type:

type Radii
    = AllCorners Float
    | DiagonalCorners { topLeft : Float, bottomRight : Float }
    | FourCornersByThreeValues  { topLeft : Float, topRightBottomLeft : Float, bottomRight : Float }
    | FourCorners { topLeft : Float, topRight : Float, bottomLeft : Float, bottomRight : Float }
    
-- plus, some helpers as well

Both variants could always translate to four-valued list.

shamansir added a commit to shamansir/elm-canvas that referenced this pull request Dec 27, 2022
@mbylstra
Copy link

@joakin Would you consider approving this PR? Although I think @shamansir's suggestions are good, I also think the list of float's API in this PR is totally adequate, not a deal breaker - not something that needs to hold up the PR for several more years.

@mbylstra
Copy link

I have tested this branch locally by linking in the elm.json "src" - it is working well

@joakin
Copy link
Owner

joakin commented Jul 26, 2024

@mbylstra sure, I'll have a look in the next two weeks, I'm on vacation without a computer right now.

The PR looks good but I'd like to specify the radii in Canvas.elm in a more type safe way like @shamansir suggested. Maybe just having the radii be a record of { topLeft, topRight, bottomLeft, bottomRight } would work fine, no need to over complicate the API.

Another option is to have roundRect take a single Float for all radii which is the most common case, and have a nother roundRectCustom or something like that that takes the record with the four corners.

@mbylstra
Copy link

Sound good! Please enjoy your vacation and take a break from coding 😄

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

Successfully merging this pull request may close these issues.

4 participants