Allow controlling where responsive utilities are rendered using optional @tailwind screens; directive
#88
Conversation
|
Uups, closed by accident |
There was a problem hiding this comment.
Hey @psren this is an awesome start, nice job! I've left a few comments about some things you'll want to tweak to get it working perfectly 👍
| .then(result => { | ||
| const expected = fs.readFileSync(path.resolve(`${__dirname}/fixtures/tailwind-output.css`), 'utf8') | ||
|
|
||
| expect(result.css).toBe(expected) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 ?
There was a problem hiding this comment.
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!!
There was a problem hiding this comment.
Okay, I'm wrong.
Everything is fine, i just had wrong expectations.
| css.walkAtRules('tailwind', atRule => { | ||
| if (atRule.params === 'screen-utilities') { | ||
| includesScreenUtilitiesExplicitly = true | ||
| atRule.replaceWith(mediaQuery) |
There was a problem hiding this comment.
Right now you're doing this inside the loop that writes each media query, so the screen-utilities marker is going to get removed on the very first run of the loop and then the rest of the media queries will be added to the end of the stylesheet again.
You'll want to do the replacement at the very end, outside of the loop, and instead of writing those media queries in the loop, you'll just want to build them up and put them all into an array so you can replace the marker with all of the media queries at once later.
There was a problem hiding this comment.
Oooops, you're right. I'll fix it as soon we have a good test.
|
|
||
| .john { | ||
| content: "wick"; | ||
| } |
There was a problem hiding this comment.
This test case is exactly what I was going to suggest! 😄 Nice work!
There was a problem hiding this comment.
:-D Testcase should be good now.
I'll make it green now ;-)
There was a problem hiding this comment.
@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.
|
|
||
| let includesScreenUtilitiesExplicitly = false | ||
| css.walkAtRules('tailwind', atRule => { | ||
| if (atRule.params === 'screen-utilities') { |
There was a problem hiding this comment.
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.
|
Just for completion: Plays very well with the following PR |
|
Hey @psren feel like changing the at-rule to |
@tailwind screens; directive
# Conflicts: # __tests__/sanity.test.js # src/lib/substituteResponsiveAtRules.js
|
@adamwathan renamed the I also added the directive to the docs, but marked as optional |
Resolves #18.
I made a PR to "fix" #18
I'm not experienced in developing for PostCSS and I do not use Javascript at all :-D
Please do not laugh and please do not hesitate to give me advice for improvement.
I'm hungry to learn and help.
I left comments in the PR to the things that look for me like code-smell.
(Okay, Javascript always looks like codesmell for me, but I marked the place where I introduced codesmell)