-
Notifications
You must be signed in to change notification settings - Fork 61
Remove selector wrap from injectGlobal #116
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great, thanks for digging into this! One comment above, why'd you remove that test?
Could also use some more specific tests, but then we should be good to ship 👌
margin: 0; | ||
} | ||
` | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That should still be an error, that's wrong indentation. Just like we don't support it with css
or styled
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh yeah, I forgot to comment on this as I wrote the code yesterday. The problem is my removeBaseIndentation
function and I don't see any good way to go around this, do you understand what I mean or should I be more explicit?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not quite, why shouldn't this throw an error anymore?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah sorry I'm also a bit jetlagged and sitting in an 8 hour orientation day at NYU but I'll do my best :P.
In order to avoid general indentation errors I need to remove all the base indentation, right? So as long as that function is being called this won't throw an error. And in order to make it throw an error we would somehow need to induce an error as if they don't indent anything like that we actually just want to straight up insert it. It's purely a Styled Components style problem, not a css problem.
I'm not saying we shouldn't, I'm saying it's maybe too hard
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In order to avoid general indentation errors I need to remove all the base indentation, right?
Is it not the goal here to have exactly the same output as what styled-components' injectGlobal would output? Because that's basically what we want to lint right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The whole point is that it looks ugly in JS! We aren't linting the resulting CSS we're linting the code the user wrote and we make sure it doesn't look ugly and has errors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's why
const Button = styled.button`
color: blue;
`
throws an error, eventhough the resulting CSS is the same it's still ugly and it's a stylelint error. That's the whole point of stylelint?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah and I get that part, and while @ismay might be making a slightly different point than mine, my reason for removing that test was simply that my only way of implementing this in a simple way broke that test, so I deleted it thinking of it as collateral damage of this PR, not because I wanted to delete it but because I "had to" (not really I guess, I haven't thought it out yet but it seems like it'd get really ugly writing that code to handle this special case, and the code could possibly be error prone as well and yeah...).
The thing is, in order to prevent all the other cases of the indentation rule breaking we need to put everything on the baseline anyway, so we'd have to somehow implement the stylelint indentation rule ourselves in order to check if before we put it on the baseline whether it has broken our semantics and then somehow put in a purposefully wrongly indented piece of css into stylelint to force it to throw an error or something like that and that get's messy reeaaaalll fast I'm afraid.
That's my side of the story.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahhh okay, that makes sense. In that case, let's log that somewhere and move on!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay haha :)
3a3424d
to
bddac5b
Compare
@mxstbr so if we're at least temporarily okay with deleting the broken test I deleted is there anything else you'd like changed specifically for you to approve the PR and we merge it in? |
Just some tests that use |
Hmmm... maybe I could add one to it's not like that's super hardcore tested, but it is in there, and I adjusted the expect x amount of warnings to our new expected behavior, so apart from maybe adding an |
Just add the code from the original issue that we're trying to fix as a test: injectGlobal`
html {
color: blue;
}
`; to make sure we don't regress 😊 That's all! |
Okay, I'll get it over and done with right away :) |
Okay, I believe this should be ready now :). There was more prettier weird errors like @mxstbr experienced recently and I also did in the start of my time contributing here, I had to do an Anyway, stuff should be good now. I'll just rebase it now to get rid of the merge conflicts. |
cf998e7
to
ac5ef7d
Compare
-0.4% coverage 😢 |
Oh yeah, I'll just pop in a unit test covering that :) |
Awesome!! Let's open an issue for that indentation thing? |
Wuhuu! Doesn't this actually mean we're ready for publishing 1.0 now? :D. We'd of course update the changelog as part of the publish, but I actually think this is all that I personally wanted fixed before 1.0 anyway. |
resolves #26.
Put this together on the plane, could possibly use a few more tests but I think it should be pretty okay?