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

Upgrade: Handle darkMode value with block syntax #16507

Merged

Conversation

philipp-spiess
Copy link
Member

Closes #16171

This PR handles darkMode variant configs containing braces (so creating sub-rules) the same way we handle it in the interop layer. Since the interop layer runs inside the addVariant API that we do not run here, I instead oped to copy the one liner.

Test plan

Updated one of the migration tests to include a rule that wasn't working before. Ensured the new output works via https://play.tailwindcss.com/nR99uhKtv3

@philipp-spiess philipp-spiess requested a review from a team as a code owner February 13, 2025 15:16
@philipp-spiess philipp-spiess force-pushed the fix/allow-curly-braces-in-js-theme-darkMode-upgrade branch from 699b486 to f4f9246 Compare February 13, 2025 15:16
}
}

return output
Copy link
Contributor

Choose a reason for hiding this comment

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

Will this actually work with an array of format strings? @custom-variant overrides

Copy link
Member Author

Choose a reason for hiding this comment

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

ok i will be less lazy

Comment on lines +219 to +223
if (variantName.includes('{')) {
customVariant += variantName.replace('}', '{ @slot }}') + '\n'
} else {
customVariant += variantName + '{ @slot }\n'
}
Copy link
Member

Choose a reason for hiding this comment

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

Not 100% if this is enough, can we add a few more test cases:

Nested media queries:

darkMode: ['variant', '@media not print { @media screen { .dark & } }'],
/* dark:flex */
@media not print {
  @media screen {
    .dark .dark\:flex {
      display: flex;
    }
  }
}

Play: https://play.tailwindcss.com/n3k65J6HJF?file=config

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

I think this should work, we decided to go with the "replaces first closing brace with the slot stuff" in the addVariant API so this just makes sure whatever we create would be equivalent to running the compat layer at this point haha

Your specific example would convert to this which would work: https://play.tailwindcss.com/94rgXzHkCF

Copy link
Member

Choose a reason for hiding this comment

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

:shipit:

@philipp-spiess philipp-spiess merged commit a1e083a into main Feb 14, 2025
5 checks passed
@philipp-spiess philipp-spiess deleted the fix/allow-curly-braces-in-js-theme-darkMode-upgrade branch February 14, 2025 15:48
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.

[v4] upgrade does not work for dark mode variant with media query
3 participants