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.

Sign up for GitHub

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

Jump to bottom

feat: inline const variables in CSS #178

Merged

itsdouges merged 15 commits into atlassian-labs:master from ankeetmaini:inline-const

May 1, 2020

Contributor

ankeetmaini commented Apr 28, 2020

Fix #167

Collaborator

itsdouges commented Apr 29, 2020 •

edited

awesome change!

you'll want to merge master back in from the postcss stuff (and you have debugger statements in your tests - i think they made CI explode hehe)

ankeetmaini force-pushed the inline-const branch from 329cd1d to 4360cf5 Compare

April 29, 2020 03:48

itsdouges reviewed

View reviewed changes

packages/ts-transform/src/utils/ast-node.tsx Outdated

@@ @@ -21,6 +21,8 @@ export const getIdentifierText = ( @@
                 return ((node as ts.Identifier).escapedText as string) || (node as ts.Identifier).text;
               };
+              export const getDeclarationValue = (node: ts.VariableDeclaration) => node.initializer?.getText();

Collaborator

itsdouges Apr 29, 2020 •

edited

if we want it without the quotes we can do (code wont be 100% correct):

export const getDeclarationValue = (node: ts.VariableDeclaration) => {
  if (ts.isLiteral(node.initializer) {
    return node.initializer.text;
  }
  
  throw createNodeError(node, 'should have been a literal node');
};

we can then remove the remove quotes func.

fyi totally recommend if you havent already to use https://ts-ast-viewer.com - helps sooo much

Contributor Author

ankeetmaini Apr 29, 2020

Thanks so much, I spent way too much time understanding the nodes and types.

itsdouges reviewed

View reviewed changes

packages/ts-transform/src/utils/template-literal-to-css.tsx Outdated

-                        cssVariableExpression = joinToBinaryExpression(
-                          ts.createStringLiteral(before.variablePrefix),
-                          span.expression
+                    if (isConst(value)) {

Collaborator

itsdouges Apr 29, 2020

we might want to move this into the isStringLiteral/isNumbericLiteral if statement - else this might be inlining object and function stuff?

Contributor Author

ankeetmaini Apr 29, 2020

got it!

Contributor Author

ankeetmaini Apr 29, 2020

This reduced the large diff too, which was coming cuz of all the nesting of if-else!

Collaborator

itsdouges Apr 29, 2020

if you turn off whitespace the diff becomes way better

Collaborator

itsdouges commented Apr 29, 2020

left some comments :) awesome first pass

itsdouges added the wip 🚧 label

ankeetmaini changed the title ~~[WIP] feat: Inline const variables in CSS~~ [WIP] feat: inline const variables in CSS

ankeetmaini changed the title ~~[WIP] feat: inline const variables in CSS~~ feat: inline const variables in CSS

ankeetmaini force-pushed the inline-const branch from ea89324 to 259385f Compare

April 29, 2020 20:06

Contributor Author

ankeetmaini commented Apr 29, 2020

@Madou this is ready for review now. :)

itsdouges reviewed

View reviewed changes

packages/ts-transform/src/class-names/__tests__/index.test.tsx Outdated

@@ @@ -332,8 +332,8 @@ describe('class names transformer', () => { @@
                       );
                     `);
-                    expect(actual).toInclude('style={{ "--var-test-fontsize20": fontSize + "px" }}');
-                    expect(actual).toInclude('.css-test{font-size:var(--var-test-fontsize20)}');
+                    expect(actual).toInclude('<div style={{}} className={"css-test"}>hello, world!</div>');

Collaborator

itsdouges Apr 29, 2020

can you update this test to use a dnamic value? maybe throw in a hook use

const [fontSize] = useState(12);

itsdouges reviewed

View reviewed changes

packages/ts-transform/src/class-names/__tests__/index.test.tsx Outdated

-                      '<div style={{ "--var-test-color": color }} className={"css-test"}>hello, world!</div>'
-                    );
+                    expect(actual).toInclude('.css-test{color:blue}');
+                    expect(actual).toInclude('<div style={{}} className={"css-test"}>hello, world!</div>');

Collaborator

itsdouges Apr 29, 2020 •

edited

can you make this test use a dynamic value?

const [color] = useState('blue');

itsdouges reviewed

View reviewed changes

packages/ts-transform/src/css-prop/__tests__/index.test.tsx Outdated

@@ @@ -89,7 +89,10 @@ describe('css prop transformer', () => { @@
                     const Component = ({ className, style }) => <div className={className} style={style} css={{ fontSize: 12, color: red }}>hello world</div>;
                   `);
-                  expect(actual).toInclude('style={{ ...style, "--var-test-red": red }}');
+                  expect(actual).toInclude(
+                    '<div className={"css-test" + (className ? " " + className : "")} style={style}>hello world</div>'

Collaborator

itsdouges Apr 29, 2020

dynamic value in test pls

itsdouges reviewed

View reviewed changes

packages/ts-transform/src/css-prop/__tests__/index.test.tsx Outdated

@@ @@ -101,7 +104,8 @@ describe('css prop transformer', () => { @@
                     const Component = ({ className, ...props }) => <div className={className} style={props.style} css={{ fontSize: 12, color: red }}>hello world</div>;
                   `);
-                  expect(actual).toInclude('style={{ ...props.style, "--var-test-red": red }}');
+                  expect(actual).toInclude('style={props.style}');

Collaborator

itsdouges Apr 29, 2020

dynamic value in test pls

itsdouges reviewed

View reviewed changes

packages/ts-transform/src/css-prop/__tests__/index.test.tsx Outdated

@@ @@ -124,7 +128,8 @@ describe('css prop transformer', () => { @@
                     const Component = ({ className, style }) => <div className={className} style={{ ...style, display: 'block' }} css={{ fontSize: 12, color: red }}>hello world</div>;
                   `);
-                  expect(actual).toInclude(`style={{ ...style, display: 'block', \"--var-test-red\": red }}`);
+                  expect(actual).toInclude(`style={{ ...style, display: 'block' }}`);

Collaborator

itsdouges Apr 29, 2020

dynamic value in test pls

itsdouges reviewed

View reviewed changes

packages/ts-transform/src/css-prop/__tests__/index.test.tsx

    
            @@ -252,9 +260,9 @@ describe('css prop transformer', () => {
          
                  `);

                  expect(actual).toInclude(

                    `background-image:linear-gradient(45deg,var(--var-test-n30gray) 25%,transparent 25%),linear-gradient(-45deg,var(--var-test-n30gray) 25%,transparent 25%),linear-gradient(45deg,transparent 75%,var(--var-test-n30gray) 75%),linear-gradient(-45deg,transparent 75%,var(--var-test-n30gray) 75%)`

                    `background-image:linear-gradient(45deg,gray 25%,transparent 25%),linear-gradient(-45deg,gray 25%,transparent 25%),linear-gradient(45deg,transparent 75%,gray 75%),linear-gradient(-45deg,transparent 75%,gray 75%)`

Collaborator

itsdouges Apr 29, 2020

can you make another test that uses dynamic values for this one please

itsdouges reviewed

View reviewed changes

packages/ts-transform/src/css-prop/__tests__/index.test.tsx

    
                    expect(actual).toInclude(

                      'background-image:linear-gradient(45deg,var(--var-test-n30gray) 25%,transparent 25%),linear-gradient(-45deg,var(--var-test-n30gray) 25%,transparent 25%),linear-gradient(45deg,transparent 75%,var(--var-test-n30gray) 75%),linear-gradient(-45deg,transparent 75%,var(--var-test-n30gray) 75%)'

                      'background-image:linear-gradient(45deg,gray 25%,transparent 25%),linear-gradient(-45deg,gray 25%,transparent 25%),linear-gradient(45deg,transparent 75%,gray 75%),linear-gradient(-45deg,transparent 75%,gray 75%)'

Collaborator

itsdouges Apr 29, 2020

can you make another test that uses dynamic values pls

Contributor Author

ankeetmaini Apr 30, 2020

Yep, I've added them now. Sorry, I totally forgot removing dynamic cases would remove tests that protect these features :)

Collaborator

itsdouges commented Apr 29, 2020

awesome work - just left some comments to add some stuff back to tests 😄

ankeetmaini mentioned this pull request

fix: destructured variable used inside string template #185

Merged

ankeetmaini added 11 commits

May 1, 2020 12:55


          Handle const vars

99067db


          Make template strings too inline consts

c86de06


          DRY and tidy the code

f21dfe3


          Improve the targetting of nodes.

ccfc3ef


          Update tests.

1a9baaa


          Undo an unintentional change

e08ef15


          Catch const higher up and use prefix/suffix

43fa039


          Fix more tests

24d7981


          Make tests pass :D

89f8e86


          Remove comments.

bc46073


          Add dynamic assertions back.

7f7d907

ankeetmaini added 2 commits

May 1, 2020 12:55


          Fix test try -1

695aaa8


          Handle literal text inside templates for BindingElement

405b645

ankeetmaini force-pushed the inline-const branch from 19cd571 to 405b645 Compare

May 1, 2020 07:25


          Remove the leftover debugger statement

Contributor Author

ankeetmaini commented May 1, 2020

@Madou I think this is ready for another review. Took longer than what I expected. templateLiteralToCss is a beast :D

itsdouges approved these changes

View reviewed changes


          Merge branch 'master' into inline-const

0ccd842

itsdouges merged commit 3261f8a into atlassian-labs:master

Collaborator

itsdouges commented May 1, 2020

love it

ankeetmaini deleted the inline-const branch

May 2, 2020 03:13

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment