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

Add overflow-hidden to Card component #1352

Closed
wants to merge 1 commit into from
Closed

Conversation

acelaya
Copy link
Contributor

@acelaya acelaya commented Nov 7, 2023

Add overflow-hidden class to Card component.

This is specially important when rounded corners are enabled, otherwise consuming code has to "work around it" by inheriting rounded corners or hiding overflow themselves.

At first I thought it would be better to not make assumptions and give as much control as possible to downstream projects, but during the implementation of hypothesis/client#5938, I have realized that you always have to end up hiding the card's overflowing content.

And in fact, it makes sense. The card is an opinionated component to "wrap" content. It defines borders and background color, so you don't want content rendering out of it.

This has become more obvious with rounded corners though, so I'm adding the overflow-hidden class.

Copy link
Member

@robertknight robertknight left a comment

Choose a reason for hiding this comment

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

The rationale makes sense. Downstream consumers will have to watch out for this though because it can clip any dialogs they might try to show:

Dialog clip

If the popover attribute was widely supported then UI elements like the above could use that to break out of overflow styling. Sadly that is still very new.

What should do we do in the interim? Require consumers to add overflow-none to override this, in which case they may have to do additional work to avoid clipping the rounded borders?

@acelaya
Copy link
Contributor Author

acelaya commented Nov 8, 2023

The rationale makes sense. Downstream consumers will have to watch out for this though because it can clip any dialogs they might try to show

Ugh, good catch.

Let me give it a though. In the meantime we can keep the code in client as is, and apply overflow-hidden where it makes sense, instead of doing it the other way around.

@acelaya acelaya marked this pull request as draft November 8, 2023 11:33
@acelaya
Copy link
Contributor Author

acelaya commented Nov 10, 2023

I have been experimenting with a couple of options that would address the rounded corner overlapping without introducing the overflow hidden side effects mentioned by Rob:

  1. Making usual Card children (mainly CardHeader and CardContent) conditionally inherit roundiness, by checking if they are the first/last children, and applying the rounded class accordingly (first-child:rounded-t-[inherit], last-child:rounded-b-[inherit], etc).
  2. Apply overflow-hidden to Card, with a mechanism to disable it and document how downstream consumers would have to deal with overlapping corners themselves.

None of them really solves the problem:

  1. It's very usual that Cards have other children, or wrap their children into something else, making inheriting rounded corners very hard to do reliably.
  2. This is technically speaking a breaking change, and would enforce checking every single usage of Card in order to check if we have to disable overflow-hidden, with the risk that we overlook some usage.

Additionally, #1358 mitigates one of the cases in which Cards end up rendering square corners.

Because of this, I have decided to close this for now, as the most pragmatic approach seems to be letting upstream code set overflow-hidden when needed.

@acelaya acelaya closed this Nov 10, 2023
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.

2 participants