-
Notifications
You must be signed in to change notification settings - Fork 67
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add CSS Map #1492
Add CSS Map #1492
Changes from all commits
664672b
73a7ffb
f5e8a25
0a22e90
184c044
8a028d5
dc772f8
c0fde8d
8002047
7c0ac9a
3f6a017
4748a80
3b6f848
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,12 @@ | ||
import { css, cssMap } from '@compiled/react'; | ||
|
||
const styles = cssMap({ | ||
danger: { | ||
color: 'red', | ||
}, | ||
success: { | ||
color: 'green', | ||
}, | ||
}); | ||
|
||
export default ({ variant, children }) => <div css={css(styles[variant])}>{children}</div>; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just walking my way through this...
const styles = css.create({
danger: {
color: 'red'
},
success: {
color: 'green'
}
});
const map = {
danger: styles.danger,
success: styles.success
}
export default ({variant, children}) => <div css={css(map[variant])} >{children}</div> Can compile to const styles = {
danger: {
color: '<hash_color_red>'
},
success: {
color: '<hash_color_green>'
}
});
const map = {
danger: styles.danger, // == {color: <hash_color_red>}
success: styles.success // == {color: <hash_color_green>}
}
export default ({variant, children}) => <div css={css(map[variant])} >{children}</div>
export default ({ extendedStyle, children }) => <div css={css(extendedStyle)}>{children}</div>; can compile to export default ({ extendedStyle, children }) => <div css={css(extendedStyle)}>{children}</div>; You're combining those two cases together... to form const styles = css.match({
danger: {
color: 'red'
},
success: {
color: 'green'
}
});
export default ({ variant, children }) => <div css={css(styles[variant])}>{children}</div>; seems like a nice shortcut to me, but I'm unsure if we need it to be a different and special construct vs what we could make possible with 1 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Compiled essentially transforms one CSS rule into three pieces.
We can conditionally apply classNames to DOM element, but sheets cannot. Hence a special construct help identifying variants (will be useful for lints too), and piece sheets together.
gets compiled to
I think the approach you mentioned from 1 is more leaning on local tranforms (which is roadmapped 🙂). If we have local transforms:
gets compiled to
We can easily use javascript to choose the style
gets compiled to
If I misunderstood the comment, let's have a quick zoom call to clarify it (maybe)? |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,12 @@ | ||
import { css, cssMap } from '@compiled/react'; | ||
|
||
const styles = cssMap({ | ||
danger: { | ||
color: 'red', | ||
}, | ||
success: { | ||
color: 'green', | ||
}, | ||
}); | ||
|
||
export default ({ variant, children }) => <div css={css(styles[variant])}>{children}</div>; |
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: moving (unless there's a reason to have it in the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it makes sense to be in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah no worries 👍 |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,199 @@ | ||
import type { TransformOptions } from '../../test-utils'; | ||
import { transform as transformCode } from '../../test-utils'; | ||
import { ErrorMessages } from '../../utils/css-map'; | ||
|
||
describe('css map behaviour', () => { | ||
beforeAll(() => { | ||
process.env.AUTOPREFIXER = 'off'; | ||
}); | ||
|
||
afterAll(() => { | ||
delete process.env.AUTOPREFIXER; | ||
}); | ||
|
||
const transform = (code: string, opts: TransformOptions = {}) => | ||
transformCode(code, { pretty: false, ...opts }); | ||
|
||
const styles = ` | ||
import { css, cssMap } from '@compiled/react'; | ||
|
||
const styles = cssMap({ | ||
danger: { | ||
color: 'red', | ||
backgroundColor: 'red' | ||
}, | ||
success: { | ||
color: 'green', | ||
backgroundColor: 'green' | ||
} | ||
}); | ||
`; | ||
|
||
describe('valid syntax', () => { | ||
it('should evaluate css map when variant is a runtime variable', () => { | ||
const actual = transform(` | ||
${styles} | ||
|
||
<div css={css(styles[variant])} />; | ||
`); | ||
|
||
expect(actual).toInclude( | ||
'<div className={ax([variant==="danger"&&"_syaz5scu _bfhk5scu",variant==="success"&&"_syazbf54 _bfhkbf54"])}/>' | ||
); | ||
}); | ||
|
||
it('should evaluate css map when variant is statically defined', () => { | ||
const actual = transform(` | ||
${styles} | ||
|
||
<div css={css(styles.success)} />; | ||
<div css={css(styles['danger'])} />; | ||
`); | ||
|
||
expect(actual).toInclude( | ||
'<div className={ax(["success"==="danger"&&"_syaz5scu _bfhk5scu","success"==="success"&&"_syazbf54 _bfhkbf54"])}/>' | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Something weird is happening here. If I do this,
I shouldn't end up with each case considering all of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yea, you are right.
instead of
Also, bundler drops the dead code |
||
); | ||
expect(actual).toInclude( | ||
'<div className={ax([\'danger\'==="danger"&&"_syaz5scu _bfhk5scu",\'danger\'==="success"&&"_syazbf54 _bfhkbf54"])}/>' | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Quoting inconsistency here |
||
); | ||
}); | ||
|
||
it('should combine CSS Map with other styles', () => { | ||
const actual = transform( | ||
` | ||
${styles} | ||
|
||
<div css={css([styles[variant], { color: 'blue' }])} />; | ||
` | ||
); | ||
|
||
expect(actual).toInclude( | ||
'<div className={ax([variant==="danger"&&"_syaz5scu _bfhk5scu",variant==="success"&&"_syazbf54 _bfhkbf54","_syaz13q2"])}/>' | ||
liamqma marked this conversation as resolved.
Show resolved
Hide resolved
|
||
); | ||
}); | ||
}); | ||
|
||
describe('invalid syntax', () => { | ||
it('does not support TemplateLiteral as object property', () => { | ||
expect(() => { | ||
transform(` | ||
${styles} | ||
|
||
<div css={css(styles[\`danger\`])} />; | ||
`); | ||
}).toThrow(ErrorMessages.VARIANT_ACCESS); | ||
}); | ||
|
||
it('does not support BinaryExpression as object property', () => { | ||
expect(() => { | ||
transform(` | ||
${styles} | ||
|
||
<div css={css(styles[isDanger?'danger':'success'])} />; | ||
`); | ||
}).toThrow(ErrorMessages.VARIANT_ACCESS); | ||
|
||
expect(() => { | ||
transform(` | ||
${styles} | ||
|
||
<div css={css(styles['dang' + 'er'])} />; | ||
`); | ||
}).toThrow(ErrorMessages.VARIANT_ACCESS); | ||
}); | ||
|
||
it('does not support MemberExpression as object property', () => { | ||
expect(() => { | ||
transform(` | ||
${styles} | ||
|
||
<div css={css(styles[props.variant])} />; | ||
`); | ||
}).toThrow(ErrorMessages.VARIANT_ACCESS); | ||
}); | ||
|
||
it('does not support CallExpression as object property', () => { | ||
expect(() => { | ||
transform(` | ||
${styles} | ||
|
||
<div css={css(styles()[variant])} />; | ||
`); | ||
}).toThrow(ErrorMessages.VARIANT_CALL_EXPRESSION); | ||
}); | ||
|
||
it('does not support nesting', () => { | ||
expect(() => { | ||
transform(` | ||
${styles} | ||
|
||
<div css={css(styles.danger.veryDanger)} />; | ||
`); | ||
}).toThrow(ErrorMessages.NESTED_VARIANT); | ||
}); | ||
|
||
it('should error out if cssMap does not receive any argument', () => { | ||
expect(() => { | ||
transform(` | ||
import { css, cssMap } from '@compiled/react'; | ||
|
||
const styles = cssMap(); | ||
|
||
<div css={css(styles.danger)} />; | ||
`); | ||
}).toThrow(ErrorMessages.NUMBER_OF_ARGUMENT); | ||
}); | ||
|
||
it('should error out if cssMap receives more than one argument', () => { | ||
expect(() => { | ||
transform(` | ||
import { css, cssMap } from '@compiled/react'; | ||
|
||
const styles = cssMap({}, {}); | ||
|
||
<div css={css(styles.danger)} />; | ||
`); | ||
}).toThrow(ErrorMessages.NUMBER_OF_ARGUMENT); | ||
}); | ||
|
||
it('should error out if cssMap does not receive an object', () => { | ||
expect(() => { | ||
transform(` | ||
import { css, cssMap } from '@compiled/react'; | ||
|
||
const styles = cssMap('color: red'); | ||
|
||
<div css={css(styles.danger)} />; | ||
`); | ||
}).toThrow(ErrorMessages.ARGUMENT_TYPE); | ||
}); | ||
|
||
it('should error out if spread element is used', () => { | ||
expect(() => { | ||
transform(` | ||
import { css, cssMap } from '@compiled/react'; | ||
|
||
const styles = cssMap({ | ||
...base | ||
}); | ||
|
||
<div css={css(styles.danger)} />; | ||
`); | ||
}).toThrow(ErrorMessages.STATIC_VARIANT_OBJECT); | ||
}); | ||
|
||
it('should error out if css map object key is dynamic', () => { | ||
expect(() => { | ||
transform(` | ||
import { css, cssMap } from '@compiled/react'; | ||
|
||
const styles = cssMap({ | ||
[variantName]: { color: 'red' } | ||
}); | ||
|
||
<div css={css(styles.danger)} />; | ||
`); | ||
}).toThrow(ErrorMessages.STATIC_VARIANT_KEY); | ||
}); | ||
}); | ||
}); |
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.
nit: Let's simplify this to
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.
Having
cssMap
simplifies types. What's the reason thatcss.create
is preferred?