Skip to content
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
@responsive {
.example {
color: red;
}
}

@tailwind screen-utilities;

.john {
content: "wick";
}
Copy link
Member

Choose a reason for hiding this comment

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

This test case is exactly what I was going to suggest! 😄 Nice work!

Copy link
Author

Choose a reason for hiding this comment

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

:-D Testcase should be good now.
I'll make it green now ;-)

Copy link
Author

@HellPat HellPat Nov 3, 2017

Choose a reason for hiding this comment

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

@adamwathan Yeah, test is green now. Could you review again and please do not hesitate to correct me again or give me hints where to improve.

Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
.example {
color: red;
}

@media (min-width: 576px) {
.sm\:example {
color: red;
}
}

@media (min-width: 768px) {
.md\:example {
color: red;
}
}

@media (min-width: 992px) {
.lg\:example {
color: red;
}
}

@media (min-width: 1200px) {
.xl\:example {
color: red;
}
}

.john {
content: "wick";
}
12 changes: 12 additions & 0 deletions __tests__/sanity.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,3 +17,15 @@ it('generates the right CSS', () => {
expect(result.css).toBe(expected)
})
})

it('generates the right CSS with implicit screen utilities', () => {
const input = fs.readFileSync(path.resolve(`${__dirname}/fixtures/tailwind-input-with-explicit-screen-utilities.css`), 'utf8')

return postcss([tailwind()])
.process(input)
.then(result => {
const expected = fs.readFileSync(path.resolve(`${__dirname}/fixtures/tailwind-output-with-explicit-screen-utilities.css`), 'utf8')

expect(result.css).toBe(expected)
Copy link
Member

Choose a reason for hiding this comment

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

This test is not quite proving what you want; try adding some extra CSS to the end of the input fixture to make sure that the responsive utilities all show up before that extra CSS and I think you'll see what I mean!

You'll need to update the output fixture too, or maybe duplicate it and add the new CSS to the end so we have separate fixtures for both scenarios.

Copy link
Author

Choose a reason for hiding this comment

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

mmhhh, the problem here is that duplicating it would mean changing anything in two places in the future. Or am I wrong?

I understood that the test does not properly test my feature. I'll hack around a bit, but duplicatiing is not a good option IMO ?

Copy link
Author

Choose a reason for hiding this comment

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

holy... this will get a mess.
When I do not duplicate everything (which is definitly bad for maintaining the library) I can't test it properly.

The problem is that the lib does not know if i used @tailwind utilities.
I changed the test to meet my expectation.

Do you have a hint how you'd tackle this?
Thanks you very much for your advice!!

Copy link
Author

Choose a reason for hiding this comment

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

Okay, I'm wrong.
Everything is fine, i just had wrong expectations.

})
})
23 changes: 19 additions & 4 deletions src/lib/substituteResponsiveAtRules.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,11 +7,12 @@ import buildMediaQuery from '../util/buildMediaQuery'
export default function(config) {
return function (css) {
const screens = config().screens
const rules = []
const responsiveRules = []
let finalRules = []

css.walkAtRules('responsive', atRule => {
const nodes = atRule.nodes
rules.push(...cloneNodes(nodes))
responsiveRules.push(...cloneNodes(nodes))
atRule.before(nodes)
atRule.remove()
})
Expand All @@ -23,13 +24,27 @@ export default function(config) {
})

mediaQuery.append(
rules.map(rule => {
responsiveRules.map(rule => {
const cloned = rule.clone()
cloned.selectors = _.map(rule.selectors, selector => `.${screen}\\:${selector.slice(1)}`)
return cloned
})
)
css.append(mediaQuery)


finalRules.push(mediaQuery)
})

let includesScreenUtilitiesExplicitly = false
css.walkAtRules('tailwind', atRule => {
if (atRule.params === 'screen-utilities') {
Copy link
Author

Choose a reason for hiding this comment

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

Do you know if it's possible to extend the "css" variable somehow to provide new methods?

css.walkTailwindRules('screen-utilities') or css.hasTailwindRules('screen-utilities') would be handy in many places. Saw this kind of checks quite a few times.

includesScreenUtilitiesExplicitly = true
atRule.replaceWith(finalRules)
}
})

if(! includesScreenUtilitiesExplicitly) {
css.append(finalRules)
}
}
}