Add placeholder mixin #42
Conversation
Add placeholder mixin for target or placeholder styling in all browsers. n/a
Docs for placeholder mixin
Add placeholder mixin for target or placeholder styling in all browsers. n/a
Current coverage is 100% (diff: 100%)@@ master #42 diff @@
===================================
Files 11 12 +1
Lines 30 31 +1
Methods 0 0
Messages 0 0
Branches 0 0
===================================
+ Hits 30 31 +1
Misses 0 0
Partials 0 0
|
Fix merge conflicts
| * | ||
| * // styled-components usage | ||
| * const div = styled.input` | ||
| * ...placeholder(styles) |
There was a problem hiding this comment.
No ..., this should be ${placeholder(cssbackground: red)}
Since css returns an Array, you'll also need to amend the types to allow for that!
There was a problem hiding this comment.
Would placeholder need its types amended though? It's css that is modifying placeholder's output to an array. If another JS to CSS library implemented another means that changed the type we would need to add that too.
There was a problem hiding this comment.
No, the input is then an array, the output is still an object.
There was a problem hiding this comment.
duh...coding past midnight is never a good idea. I'll fix this in the morning.
There was a problem hiding this comment.
@mxstbr just got back on this. I want to make sure I understand this situation, because it is more than just amending the types from what I can see. Here is what I'm seeing:
const styles = {
"background": "blue",
"color": "red"
};
return css`${styles}`This returns
[
"background: blue; color: red;"
]That means we would be passing placeholder an array with a single item that contains all the styles passed into it. Is that correct?
If that is the case I will need to refactor this, as spreading the passed in styles would not accommodate this use case, especially since the goal is to return an css as js object.
There was a problem hiding this comment.
That should totally work for styled components:
const Box = styled.div`
${placeholder(css`styles`)}
`We support arrays too, so this should work perfectly fine if just passed through, but please try it out!
When we originally made the list I had thrown together a bunch of prefixers for testing and came across a few that did not catch this or |
|
Okay, that sounds reasonable! Thanks for checking that! |
Add placeholder mixin for target or placeholder styling in all browsers. n/a
Docs for placeholder mixin
Add placeholder mixin for target or placeholder styling in all browsers. n/a
Update example, merge conflicts
Merge conflicts
|
@mxstbr this is ready...again 😉 |
Add placeholder mixin