palantir / blueprint

A React-based UI toolkit for the web
https://blueprintjs.com/
Apache License 2.0
20.71k stars 2.17k forks source link

Card border behavior differs on light/dark theme #5312

Open dantmnf opened 2 years ago

dantmnf commented 2 years ago

Environment

Code Sandbox

Link to a minimal repro (fork this code sandbox): https://codesandbox.io/s/blueprint-sandbox-forked-v3oeit?file=/src/index.tsx

Steps to reproduce

  1. create a light card and a dark card with the same content
  2. compare the size of two cards

Actual behavior

The light card has an "outset" border and the dark card has an "inset" border, resulting in different visual sizes. image

Expected behavior

The light card and the dark card should have the same visual size.

Possible solution

Use inset border in $pt-elevation-shadow-*

mud commented 2 years ago

Just noticed this myself while debugging an issue where our borders seem to disappear in dark mode.

Because dark mode uses an inset box-shadow if you add a Card with padding: 0 and a child with a background-color, the border disappears. Is there a reason why dark mode uses inset while standard mode doesn't?

adidahiya commented 2 years ago

Because dark mode uses an inset box-shadow if you add a Card with padding: 0 and a child with a background-color, the border disappears. Is there a reason why dark mode uses inset while standard mode doesn't?

@mud I think in this case you should be changing the background color of the Card itself, rather than trying to fit a child element in there with a custom background color.

Looking back at https://github.com/palantir/blueprint/pull/5278, I'm pretty sure we went with inset "elevation" borders in dark theme to achieve a consistent white border appearance for those elements (Cards, Popovers, Toasts, etc.) and make them visually "pop out" from the content underneath. We might consider revisiting this decision, but I'm not yet convinced that the 1px visual size difference is serious enough of an issue to warrant a style change.

mud commented 2 years ago

@adidahiya thanks for the context.

I just noticed this since we just upgraded to BP4. We relied on Card providing bordered containers for a bunch of components, and some had child elements with background color applied (eg. header for a list.) I'll migrate off of using Card in those cases.

adidahiya commented 2 years ago

child elements with background color applied (eg. header for a list.)

You may not have to migrate away from Card... can you try adding some styles like border: solid 1px inset $pt-dark-divider-white; border-bottom: none in those cases? I understand it feels like a bit of a hack to have to work around the Card border implementation, but it may be an good enough option for your use case.

mud commented 2 years ago

@adidahiya I ended up creating a mixin that removes the original inset box-shadow and applies a normal one. There were only a few places this was an issue for us so this is manageable for now.