Add display-mode variants for pwa#20199
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
WalkthroughThis pull request adds six new display-mode variants to TailwindCSS: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Confidence Score: 3/5The change introduces a naming conflict that would force a breaking rename later if Tailwind adds a The packages/tailwindcss/src/variants.ts — the six new variant names need reconsideration before merging. Reviews (1): Last reviewed commit: "Add newer spec values" | Re-trigger Greptile |
| staticVariant('standalone', ['@media (display-mode: standalone)']) | ||
| staticVariant('minimal-ui', ['@media (display-mode: minimal-ui)']) | ||
| staticVariant('browser', ['@media (display-mode: browser)']) | ||
| staticVariant('fullscreen', ['@media (display-mode: fullscreen)']) |
There was a problem hiding this comment.
fullscreen name will collide with a :fullscreen pseudo-class variant
fullscreen already exists as a widely-supported CSS pseudo-class (:fullscreen from the Fullscreen API). These two concepts are semantically different: @media (display-mode: fullscreen) detects a PWA installed in fullscreen mode, while :fullscreen targets any element that entered fullscreen via element.requestFullscreen(). If Tailwind later adds the :fullscreen pseudo-class variant—which is a natural addition—both would claim the fullscreen: prefix, causing a breaking conflict. The other display-mode variants (following the pointer-none / any-pointer-coarse precedent) suggest a display-mode-fullscreen name would be unambiguous.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| staticVariant('standalone', ['@media (display-mode: standalone)']) | ||
| staticVariant('minimal-ui', ['@media (display-mode: minimal-ui)']) | ||
| staticVariant('browser', ['@media (display-mode: browser)']) | ||
| staticVariant('fullscreen', ['@media (display-mode: fullscreen)']) | ||
| staticVariant('picture-in-picture', ['@media (display-mode: picture-in-picture)']) | ||
| staticVariant('window-controls-overlay', ['@media (display-mode: window-controls-overlay)']) |
There was a problem hiding this comment.
Naming convention diverges from existing multi-value media feature pattern
Existing multi-value media features prefix variant names with the property: pointer-none, pointer-coarse, pointer-fine, any-pointer-none, etc. These display-mode variants break that pattern by using bare values (standalone, browser, fullscreen). Following the same convention would produce display-mode-standalone, display-mode-browser, etc., which avoids collision with other uses of generic terms like browser or standalone and makes it immediately clear these are media-feature variants rather than pseudo-class or state variants.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Summary
Add display-mode variants to allow developer of PWA to create media query based on the display-mode of the user.
Relates to: #19698, #10194, #6755, #4955
Test plan
I followed https://github.com/tailwindlabs/tailwindcss/blob/main/.github/CONTRIBUTING.md.
I installed the rust toolchain and build the binary.
I ran
pnpm tddand all test run fine (except those that were already failing onmain).I ran
pnpm build && pnpm viteand tested in the playground.