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

Accept higher value in border-radius to be able to render a pill shaped rectangle #1006

Merged
merged 11 commits into from
Apr 20, 2023

Conversation

Poivey
Copy link
Contributor

@Poivey Poivey commented Mar 9, 2023

Summary

Remove the higher limit of border-radius, to be able to render rectangle shape as "pill".

Details

  • Computing the value of rx and ry from the shape width and height to render the same way CSS does
  • Limiting rx and ry values to half of the minumum value between width and height to mimic CSS (see 1st and 2nd example)

Examples

CSS example for reference

Screen.Recording.2023-04-13.at.16.21.36.mov

^ Note that border-radius: 50px is the same render as border-radius: 999px

Rendered SVG example

Screen.Recording.2023-04-13.at.16.22.08.mov

^ Note that rx="50" is NOT the same render as rx="999" -> This is why we need the method calculateAxisRadius to limit the value rx and ry can have.

ThisIsElement.style.border-radius: 999
ThisIsElement.width: 200
ThisIsElement.height: 100

Also works vertically

Screenshot 2023-04-13 at 16 13 15

^ When only rx is set, it the same as if ry had the same value.

Does not need width and height to be set manually to work

Screenshot 2023-04-13 at 16 55 20

ThisIsElement.style.border-radius: 999

TODO

  • Remove "percentage border-radius" tests
  • Add new tests
[Deprecated] Old description percentage border-radius solution

Summary

Considers border-radius value between (0,1) as a percentage, applied to rx and ry shape attributes.

Details

  • Computing the value of rx and ry from the shape width and height to rendre the same way CSS does
  • Add tests to check border-radius value validation
  • Add a test with a percentage border-radius
  • Some test were updated because ry was added to shape having a rx. The rendesr are the same. rx and ry will have the same value if a "number" value is given to border-radius, and might have a different one if a "percent" value is given.

Examples

Multiple shape with border-radius 10%

Html/css with border-radius 10%, it is the same shape

A shape with border-radius 35% and the same shape using html/css with border-radius 35%

Border-radius 50%

[Deprecated] Old description with first questions

Here are example of how it renders, shaped are named according the the applied border-radius value (eg. Example0,05 has border-radius: 0.05, rx will be "5%")
Screenshot
Screenshot
Screenshot
Screenshot
Screenshot

Question

I feel like the first 4 examples are rendering as expected, but the last one highlight an issue present when the image is too wide -> small percentages are rendering a more rounded border radius as expected. The difference is easily visible on both Element0,05 and ElementWithALongerName0,05 in exemple 2 and 5.
So I think the percentage value might depend on the image width. I'm not used to this so I plan to look for a solution next time I can focus on this issue (likely next week).
Do you have an idea about how I could fix this issue ? Maybe compute an integer rx value from the percentage value and the shape width instead of setting with the percentage value directly ?

FIX #947 -> Canceled as render renders were not satisfying
FIX #948

Copy link
Collaborator

@alixander alixander left a comment

Choose a reason for hiding this comment

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

@Poivey it should follow CSS conventions as much as possible: https://developer.mozilla.org/en-US/docs/Web/CSS/border-radius

you might need to introduce ry to rects too.

so that 0.25 would be 25% of x axis for rx and 25% of y axis for ry. at least i think that's how CSS is doing it

d2graph/d2graph.go Outdated Show resolved Hide resolved
d2themes/element.go Outdated Show resolved Hide resolved
d2exporter/export.go Outdated Show resolved Hide resolved
@Poivey Poivey requested a review from alixander March 17, 2023 18:49
@Poivey Poivey marked this pull request as ready for review April 6, 2023 17:21
Copy link
Collaborator

@alixander alixander left a comment

Choose a reason for hiding this comment

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

Screen Shot 2023-04-06 at 3 27 49 PM

Screen Shot 2023-04-06 at 3 30 25 PM

hm yeah these just look weird, i don't think anyone would want to use that. i guess what i really wanted was a way to create the "pill" shape effect. sorry about this dragged-out experience @Poivey . i rly appreciate this high quality investigating and work and how thorough you've been in describing the issues, but i think the end result just didn't turn out to be something desirable, aesthetically.

i also don't want to have a separate pill: true, or shape: pill though. i kind of wish the border-radius: 999 would work, like it does in CSS. up to you if you want to keep going to find the implementation for that, otherwise i can just create an issue for it separately.

@alixander
Copy link
Collaborator

alixander commented Apr 6, 2023

For exemple one solution is to set rx to 50% of the height of the rectangle

oh, what if % only applies to rx then, and just not affect the ry? that'd be a clear use case for %, and i think we don't lose anything since % just doesn't look good when applied to both rx and ry

@Poivey
Copy link
Contributor Author

Poivey commented Apr 7, 2023

up to you if you want to keep going to find the implementation for that

Sure I would like to continue, I'll have time to work on it next week.

As I understand : the goal is to have a rectangle ressemble a pill shape if given a border-radius between 0 and 1. The rectangle will be more or less "pill-like" according to the value of border-radius.

Can you confirm this is what needs to be done ?

what if % only applies to rx then, and just not affect the ry? that'd be a clear use case for %, and i think we don't lose anything since % just doesn't look good when applied to both rx and ry

Yes, from what I saw when I experimented, it's possible to have a pill shape like this. For example, if a rectangle has a height of 50 and a width of 100, to be a pill shape it would need to have rx set to 25 (50% of the height).

It's not the same as setting rx to 50%, as it is calculated on the width by default 😄
Setting only rx is the same as setting rx and ry to the same value, so in this case only rx will be needed

@alixander
Copy link
Collaborator

the goal is to have a rectangle ressemble a pill shape if given a border-radius between 0 and 1.

No I think we want to just emulate CSS's handling. Users can specify a large value for "pill". you'll probably have to increase the allowed value, which is a todo anyway (#948)

https://stackoverflow.com/questions/71023962/how-to-make-a-pill-shape-button-with-html-and-css

https://stackoverflow.com/questions/31617136/avoid-elliptical-shape-in-css-border-radius
"If the value exceeds half of the shortest side's length, the browser will use the minimum as its border-radius in both directions, producing a perfect pill shape on rectangular elements."

…lue and limit it to half of the smaller shape side to be rendered as a pill
@Poivey Poivey changed the title wip: shape border-radius with percentage value wip: Accept higher value in border-radius to be able to render a pill shaped rectangle Apr 13, 2023
@Poivey
Copy link
Contributor Author

Poivey commented Apr 13, 2023

I updated this PR title and description with new examples. Now the renders behaves like CSS does when passing a high border-radius value (see examples in description). Is it how you expected it to be ?

I will update the tests soon

Now that we dropped the idea of percentage border-radius as it did not render as expected, do you think #947 should be closed, and #948 should be assigned to me through this PR, or closed and create a new issue for pill render for this PR ?

@Poivey Poivey requested a review from alixander April 13, 2023 15:08
Copy link
Collaborator

@alixander alixander left a comment

Choose a reason for hiding this comment

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

💯 beautiful docs, tests, and code. minor nitpicks, but other than that + merge conflicts, good to go!

d2themes/element.go Outdated Show resolved Hide resolved
d2themes/element.go Outdated Show resolved Hide resolved
@alixander
Copy link
Collaborator

friendly ping @Poivey , would love to get this in

@Poivey
Copy link
Contributor Author

Poivey commented Apr 20, 2023

Thanks for the review, I'll make the change and resolve conflicts in a few hours, it should be good to go very soon

@Poivey Poivey changed the title wip: Accept higher value in border-radius to be able to render a pill shaped rectangle Accept higher value in border-radius to be able to render a pill shaped rectangle Apr 20, 2023
@Poivey
Copy link
Contributor Author

Poivey commented Apr 20, 2023

@alixander It's ready :)

@alixander
Copy link
Collaborator

thanks again!

@alixander alixander merged commit 20c9452 into terrastruct:master Apr 20, 2023
2 checks passed
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.

increase range for border-radius use 0-1 decimal value as percentages for border-radius
2 participants