Skip to content

Conversation

@thecrypticace
Copy link
Contributor

@thecrypticace thecrypticace commented Nov 20, 2024

This PR updates the default drop-shadow-* values to use a single shadow instead of multiple shadows.

This ensures that the usage with drop-shadow(var(--drop-shadow-xl)) is correct because the drop-shadow(…) needs to encode a single drop shadow.

@thecrypticace thecrypticace marked this pull request as ready for review November 20, 2024 22:08
@thecrypticace thecrypticace requested a review from a team as a code owner November 20, 2024 22:08
Copy link
Member

@RobinMalfait RobinMalfait left a comment

Choose a reason for hiding this comment

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

I think we need to do the same in here:
image

Otherwise this won't work. Let's add drop-shadow to the list of tests.

diff --git a/packages/tailwindcss/src/utilities.test.ts b/packages/tailwindcss/src/utilities.test.ts
index b16c6f125..e6120023c 100644
--- a/packages/tailwindcss/src/utilities.test.ts
+++ b/packages/tailwindcss/src/utilities.test.ts
@@ -13639,6 +13639,7 @@ test('filter', async () => {
         'invert',
         'invert-0',
         'invert-[var(--value)]',
+        'drop-shadow',
         'drop-shadow-xl',
         'drop-shadow-[0_0_red]',
         'saturate-0',
@@ -13692,6 +13693,11 @@ test('filter', async () => {
       filter: var(--tw-blur, ) var(--tw-brightness, ) var(--tw-contrast, ) var(--tw-grayscale, ) var(--tw-hue-rotate, ) var(--tw-invert, ) var(--tw-saturate, ) var(--tw-sepia, ) var(--tw-drop-shadow, );
     }
 
+    .drop-shadow {
+      --tw-drop-shadow: drop-shadow(var(--drop-shadow));
+      filter: var(--tw-blur, ) var(--tw-brightness, ) var(--tw-contrast, ) var(--tw-grayscale, ) var(--tw-hue-rotate, ) var(--tw-invert, ) var(--tw-saturate, ) var(--tw-sepia, ) var(--tw-drop-shadow, );
+    }
+
     .drop-shadow-\\[0_0_red\\] {
       --tw-drop-shadow: drop-shadow(0 0 red);
       filter: var(--tw-blur, ) var(--tw-brightness, ) var(--tw-contrast, ) var(--tw-grayscale, ) var(--tw-hue-rotate, ) var(--tw-invert, ) var(--tw-saturate, ) var(--tw-sepia, ) var(--tw-drop-shadow, );

@RobinMalfait
Copy link
Member

Can't add a suggestion/comment on untouched lines so had to do it this way 😅

thecrypticace and others added 5 commits November 20, 2024 17:31
We updated the drop shadow values to have a single shadow instead of
multiple values which allows us to keep using the `var(…)` instead of
inlining it.

The caveat is that if you define a custom drop shadow that contains
multiple values then that won't be valid CSS because the two (or more)
drop shadows will be wrapped in a `drop-shadow(shadow-1, shadow-2, …)`
Copy link
Member

@philipp-spiess philipp-spiess left a comment

Choose a reason for hiding this comment

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

Change looks good but need to update the PR description 🙈

@RobinMalfait RobinMalfait merged commit 4f63a5a into next Nov 21, 2024
1 check passed
@RobinMalfait RobinMalfait deleted the fix/v4-inline-drop-shadow-theme-values branch November 21, 2024 12:13
@thecrypticace thecrypticace changed the title Inline theme values in drop-shadow Update default drop-shadow-* values to use a single shadow Nov 21, 2024
@dmitrc
Copy link

dmitrc commented Jan 25, 2025

@thecrypticace, given the new format for generated drop-shadow filters embeds just a single shadow via CSS var, what is the guidance for setting up utilities with multiple shadows?

Is my understanding correct that you can't really do it via @theme anymore and need to roll an entirely custom @utility including all of the filter boilerplate?

@theme {
  --drop-shadow-*: initial;
  --custom-drop-shadow-foo: drop-shadow(...) drop-shadow(...);
}

@utility drop-shadow-* {
  --tw-drop-shadow: --value(--custom-drop-shadow-*);
  filter: var(--tw-blur,) var(--tw-brightness,) var(--tw-contrast,)
    var(--tw-grayscale,) var(--tw-hue-rotate,) var(--tw-invert,)
    var(--tw-saturate,) var(--tw-sepia,) var(--tw-drop-shadow,);
}

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.

5 participants