-
-
Notifications
You must be signed in to change notification settings - Fork 56
[pigment-core] React implementation for the new Pigment CSS API #367
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
base: master
Are you sure you want to change the base?
Conversation
brijeshb42
commented
Jan 14, 2025
- I have followed (at least) the PR section of the contributing guide.
493fe2c to
534e9c4
Compare
534e9c4 to
eb5c3c5
Compare
b6cb7ee to
9d6424d
Compare
9d6424d to
0c8a1c0
Compare
6e259a9 to
a0d632c
Compare
77a085d to
155b9ef
Compare
f55bf27 to
7eecf99
Compare
7eecf99 to
2f1a12d
Compare
2f1a12d to
b900094
Compare
| ? T | ||
| : never; | ||
|
|
||
| function handleTemplateElementOrSimilar( |
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.
This is the test for the change in implementation.
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.
I'll add the implementation for sx in a separate PR. At the current stage, the new package doesn't support sx prop.
60368c1 to
5a8e8c8
Compare
| @@ -0,0 +1,152 @@ | |||
| { | |||
| "name": "@pigment-css/react-new", | |||
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.
Hey there. Thanks for your work. A question about this package name, are we going to remove the -new before it is released?
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.
Yes. Its temporary right now. Before release, this'll become @pigment-css/react and old one will be renamed.
5a8e8c8 to
5e57953
Compare
| const { themeArgs = {}, pigmentFeatures: { useLayer = true } = {} } = | ||
| processor.options as TransformedInternalConfig; | ||
| // @ts-ignore @TODO - Fix this. No idea how to initialize a Tagged String array. | ||
| const templateStrs: string[] = []; |
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.
const x: TemplateStringsArray = Object.assign([], { raw: [] })|
|
||
| isMaybeTransformedTemplateLiteral(values: ValueCache): boolean { | ||
| const [, firstArg, ...restArgs] = this.callParam; | ||
| if (!('kind' in firstArg) || firstArg.kind === ValueType.CONST) { |
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.
I don't think I've used the in operator in over a decade 😄
It's purely stylistic in this instance, .hasOwnProperty feels more predictable to me.
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.
I think in makes TS happy too, not sure about .hasOwnProperty.
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.
Correct. in narrows the types to the expected ones.
| @@ -0,0 +1,29 @@ | |||
| # Pigment CSS | |||
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.
Content like this would be useful on the docs. Like a landing page with a bit more information.
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.
|
|
||
| const { result: baseObj, variables } = processStyle(style, { getVariableName }); | ||
| const cssText = serializeStyles([baseObj as any]).styles; | ||
| const { styles: cssText } = serializeStyles([baseObj as any]); |
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.
Casting type to serializeStyles's input argument would be better than any.