Repository: tailwindlabs/tailwindcss

Problem statement:
State Variant with Responsive Prefix does not work with group-hover
## Steps to reproduce
### Create element
Create an  element with:
```html
<div class="group bg-red p-8">
    <div class="hidden md:group-hover:block bg-blue p-4">
        Only visible when group is hovered
    </div>
</div>
```

### Enable `group-hover` state variant
Enable the `group-hover` state variant to the `display` module and compile
```javascript
module.exports = {
    // ...
    
    modules: {
        // ...
        
        display: ['responsive', 'group-hover'],
        
        // ...
    }
    
    // ...
}l
```

### Test
Test. It won't work. It only works without the responsive prefix: `group-hover:block`


Hints:
*Copying this over from duplicate issue #468 so I can close that one.*

---

Right now if you have `responsive` and `group-hover` variants enabled for a module, you'll get selectors like:

```
@media (min-width: 768px) {
  .md\:group:hover .group-hover\:bg-blue {
    background-color: blue
  }
}
```

I think I expect this instead:

```
@media (min-width: 768px) {
  .group:hover .md\:group-hover\:bg-blue {
    background-color: blue
  }
}
```

...or maybe even this:

```
@media (min-width: 768px) {
  .md\:group:hover .md\:group-hover\:bg-blue {
    background-color: blue
  }
}
```

This turns out to be really hard to solve based on how we currently generate responsive variants — it's a second step later in the build after generating regular variants.

I'm not sure what the best solution is here, but it reminded me that I've been thinking about making the "responsive + state" variants more explicit, so I wanted to document that idea for future reference.

Basically, instead of this automatically generating responsive hover utilities:

```
backgroundColors: ['responsive', 'hover'],
```

...we'd add a third variant you could use to generate them explicitly:

```
backgroundColors: ['responsive', 'hover', 'responsive-hover'],
```

This should give us more control over how responsive variants are generated for each existing variant type, making it easier to solve the `responsive-group-hover` problem.

It would be a breaking change but not too annoying to update for; you'd just need to update the variant lists in the `modules` section of your config to add all of the new explicit variants.

I'm thinking I'd make it the job of each variant to generate the media query + completed class name, and add a new step in the build where all of the media queries get grouped, rather than generating the media queries and grouping them in the same step. That would give each variant generator complete control over how the responsive selectors are generated.

I'll have to think through if it makes sense to group all media queries (even those written by the user and not generated by Tailwind); I'm thinking "no" but if that's the case I'll need to come up with some sort of special internal marker we can use to mark the media queries that should be grouped.


Patch:
diff --git a/src/lib/substituteResponsiveAtRules.js b/src/lib/substituteResponsiveAtRules.js
--- a/src/lib/substituteResponsiveAtRules.js
+++ b/src/lib/substituteResponsiveAtRules.js
@@ -2,7 +2,7 @@ import _ from 'lodash'
 import postcss from 'postcss'
 import cloneNodes from '../util/cloneNodes'
 import buildMediaQuery from '../util/buildMediaQuery'
-import buildClassVariant from '../util/buildClassVariant'
+import buildSelectorVariant from '../util/buildSelectorVariant'
 
 export default function(config) {
   return function(css) {
@@ -28,7 +28,9 @@ export default function(config) {
         responsiveRules.map(rule => {
           const cloned = rule.clone()
           cloned.selectors = _.map(rule.selectors, selector =>
-            buildClassVariant(selector, screen, separator)
+            buildSelectorVariant(selector, screen, separator, message => {
+              throw rule.error(message)
+            })
           )
           return cloned
         })
diff --git a/src/lib/substituteVariantsAtRules.js b/src/lib/substituteVariantsAtRules.js
--- a/src/lib/substituteVariantsAtRules.js
+++ b/src/lib/substituteVariantsAtRules.js
@@ -1,9 +1,9 @@
 import _ from 'lodash'
 import postcss from 'postcss'
-import buildClassVariant from '../util/buildClassVariant'
+import buildSelectorVariant from '../util/buildSelectorVariant'
 
 function buildPseudoClassVariant(selector, pseudoClass, separator) {
-  return `${buildClassVariant(selector, pseudoClass, separator)}:${pseudoClass}`
+  return `${buildSelectorVariant(selector, pseudoClass, separator)}:${pseudoClass}`
 }
 
 function generatePseudoClassVariant(pseudoClass) {
@@ -23,7 +23,11 @@ const variantGenerators = {
     const cloned = container.clone()
 
     cloned.walkRules(rule => {
-      rule.selector = `.group:hover ${buildClassVariant(rule.selector, 'group-hover', separator)}`
+      rule.selector = `.group:hover ${buildSelectorVariant(
+        rule.selector,
+        'group-hover',
+        separator
+      )}`
     })
 
     container.before(cloned.nodes)
diff --git a/src/util/buildClassVariant.js b/src/util/buildClassVariant.js
deleted file mode 100644
--- a/src/util/buildClassVariant.js
+++ /dev/null
@@ -1,5 +0,0 @@
-import escapeClassName from './escapeClassName'
-
-export default function buildClassVariant(className, variantName, separator) {
-  return `.${variantName}${escapeClassName(separator)}${className.slice(1)}`
-}
diff --git a/src/util/buildSelectorVariant.js b/src/util/buildSelectorVariant.js
new file mode 100644
--- /dev/null
+++ b/src/util/buildSelectorVariant.js
@@ -0,0 +1,16 @@
+import escapeClassName from './escapeClassName'
+import parser from 'postcss-selector-parser'
+import tap from 'lodash/tap'
+
+export default function buildSelectorVariant(selector, variantName, separator, onError = () => {}) {
+  return parser(selectors => {
+    tap(selectors.first.filter(({ type }) => type === 'class').pop(), classSelector => {
+      if (classSelector === undefined) {
+        onError('Variant cannot be generated because selector contains no classes.')
+        return
+      }
+
+      classSelector.value = `${variantName}${escapeClassName(separator)}${classSelector.value}`
+    })
+  }).processSync(selector)
+}


Test patch:
diff --git a/__tests__/responsiveAtRule.test.js b/__tests__/responsiveAtRule.test.js
new file mode 100644
--- /dev/null
+++ b/__tests__/responsiveAtRule.test.js
@@ -0,0 +1,245 @@
+import postcss from 'postcss'
+import plugin from '../src/lib/substituteResponsiveAtRules'
+import config from '../defaultConfig.stub.js'
+
+function run(input, opts = () => config) {
+  return postcss([plugin(opts)]).process(input, { from: undefined })
+}
+
+test('it can generate responsive variants', () => {
+  const input = `
+    @responsive {
+      .banana { color: yellow; }
+      .chocolate { color: brown; }
+    }
+  `
+
+  const output = `
+      .banana { color: yellow; }
+      .chocolate { color: brown; }
+      @media (min-width: 500px) {
+        .sm\\:banana { color: yellow; }
+        .sm\\:chocolate { color: brown; }
+      }
+      @media (min-width: 750px) {
+        .md\\:banana { color: yellow; }
+        .md\\:chocolate { color: brown; }
+      }
+      @media (min-width: 1000px) {
+        .lg\\:banana { color: yellow; }
+        .lg\\:chocolate { color: brown; }
+      }
+  `
+
+  return run(input, () => ({
+    screens: {
+      sm: '500px',
+      md: '750px',
+      lg: '1000px',
+    },
+    options: {
+      separator: ':',
+    },
+  })).then(result => {
+    expect(result.css).toMatchCss(output)
+    expect(result.warnings().length).toBe(0)
+  })
+})
+
+test('it can generate responsive variants with a custom separator', () => {
+  const input = `
+    @responsive {
+      .banana { color: yellow; }
+      .chocolate { color: brown; }
+    }
+  `
+
+  const output = `
+      .banana { color: yellow; }
+      .chocolate { color: brown; }
+      @media (min-width: 500px) {
+        .sm__banana { color: yellow; }
+        .sm__chocolate { color: brown; }
+      }
+      @media (min-width: 750px) {
+        .md__banana { color: yellow; }
+        .md__chocolate { color: brown; }
+      }
+      @media (min-width: 1000px) {
+        .lg__banana { color: yellow; }
+        .lg__chocolate { color: brown; }
+      }
+  `
+
+  return run(input, () => ({
+    screens: {
+      sm: '500px',
+      md: '750px',
+      lg: '1000px',
+    },
+    options: {
+      separator: '__',
+    },
+  })).then(result => {
+    expect(result.css).toMatchCss(output)
+    expect(result.warnings().length).toBe(0)
+  })
+})
+
+test('responsive variants are grouped', () => {
+  const input = `
+    @responsive {
+      .banana { color: yellow; }
+    }
+
+    .apple { color: red; }
+
+    @responsive {
+      .chocolate { color: brown; }
+    }
+  `
+
+  const output = `
+      .banana { color: yellow; }
+      .apple { color: red; }
+      .chocolate { color: brown; }
+      @media (min-width: 500px) {
+        .sm\\:banana { color: yellow; }
+        .sm\\:chocolate { color: brown; }
+      }
+      @media (min-width: 750px) {
+        .md\\:banana { color: yellow; }
+        .md\\:chocolate { color: brown; }
+      }
+      @media (min-width: 1000px) {
+        .lg\\:banana { color: yellow; }
+        .lg\\:chocolate { color: brown; }
+      }
+  `
+
+  return run(input, () => ({
+    screens: {
+      sm: '500px',
+      md: '750px',
+      lg: '1000px',
+    },
+    options: {
+      separator: ':',
+    },
+  })).then(result => {
+    expect(result.css).toMatchCss(output)
+    expect(result.warnings().length).toBe(0)
+  })
+})
+
+test('screen prefix is only applied to the last class in a selector', () => {
+  const input = `
+    @responsive {
+      .banana li * .sandwich #foo > div { color: yellow; }
+    }
+  `
+
+  const output = `
+      .banana li * .sandwich #foo > div { color: yellow; }
+      @media (min-width: 500px) {
+        .banana li * .sm\\:sandwich #foo > div { color: yellow; }
+      }
+      @media (min-width: 750px) {
+        .banana li * .md\\:sandwich #foo > div { color: yellow; }
+      }
+      @media (min-width: 1000px) {
+        .banana li * .lg\\:sandwich #foo > div { color: yellow; }
+      }
+  `
+
+  return run(input, () => ({
+    screens: {
+      sm: '500px',
+      md: '750px',
+      lg: '1000px',
+    },
+    options: {
+      separator: ':',
+    },
+  })).then(result => {
+    expect(result.css).toMatchCss(output)
+    expect(result.warnings().length).toBe(0)
+  })
+})
+
+test('responsive variants are generated for all selectors in a rule', () => {
+  const input = `
+    @responsive {
+      .foo, .bar { color: yellow; }
+    }
+  `
+
+  const output = `
+      .foo, .bar { color: yellow; }
+      @media (min-width: 500px) {
+        .sm\\:foo, .sm\\:bar { color: yellow; }
+      }
+      @media (min-width: 750px) {
+        .md\\:foo, .md\\:bar { color: yellow; }
+      }
+      @media (min-width: 1000px) {
+        .lg\\:foo, .lg\\:bar { color: yellow; }
+      }
+  `
+
+  return run(input, () => ({
+    screens: {
+      sm: '500px',
+      md: '750px',
+      lg: '1000px',
+    },
+    options: {
+      separator: ':',
+    },
+  })).then(result => {
+    expect(result.css).toMatchCss(output)
+    expect(result.warnings().length).toBe(0)
+  })
+})
+
+test('selectors with no classes cannot be made responsive', () => {
+  const input = `
+    @responsive {
+      div { color: yellow; }
+    }
+  `
+  expect.assertions(1)
+  return run(input, () => ({
+    screens: {
+      sm: '500px',
+      md: '750px',
+      lg: '1000px',
+    },
+    options: {
+      separator: ':',
+    },
+  })).catch(e => {
+    expect(e).toMatchObject({ name: 'CssSyntaxError' })
+  })
+})
+
+test('all selectors in a rule must contain classes', () => {
+  const input = `
+    @responsive {
+      .foo, div { color: yellow; }
+    }
+  `
+  expect.assertions(1)
+  return run(input, () => ({
+    screens: {
+      sm: '500px',
+      md: '750px',
+      lg: '1000px',
+    },
+    options: {
+      separator: ':',
+    },
+  })).catch(e => {
+    expect(e).toMatchObject({ name: 'CssSyntaxError' })
+  })
+})