Skip to content
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

Improvements to parsing, code completion, inspections #7

Merged
merged 2 commits into from
Aug 31, 2017

Conversation

undeadcat
Copy link
Contributor

Hi!
I'm from the WebStorm team at JetBrains. Thank you for creating this plugin! We really appreciate your efforts!

We were actually planning to release a similar plugin to support Styled Components within the nearest days when we saw this plugin.
We would love it if you could accept our implementation that solves some of the issues you've mentioned in the readme and join the efforts with us to make support for Styled Components in WebStorm amazing.

Here's how the implementation in this PR is different:

  1. When building injected content we replace the string interpolation argument with a fake identifier to avoid breaking the CSS syntax structure as much is possible.
    For example, in case of
let someCss = styled(Box) `
	color: red;
        ${getNestedSelector()} {
	    color: green
	}`;

the current implementation would generate

sel {
  color: red;
}

sel {
  fakeprop: initial {
  color: green
} initial;
}

which places the elements that should be nested side-by-side, significantly changing the code semantics.
This new version generates

div {
  color: red;
  EXTERNAL_FRAGMENT {
    color: green
  }
}

This keeps nested elements nested (as they should be) and also allows code completion to work correctly.
image

  1. Added tests that check injected content and for Expression inside parenthesis breaks parsing #6

  2. Set the minumum compatible IDE version to 2017.2 (2017.2 AND all later versions should be supported). Reason for this is there were API changes in 2017.1, I think it may be made to work with earlier versions, but not sure it's worth it without a specific need.

The code is written in Kotlin and uses intellij-gradle-plugin to build it but this is only because our code was originally written this way.
I really don't have a preference here and since this is your project, I'll gladly convert it to Java or use your preferred build tool if you do =)

* use dummy identifier (EXTERNAL_FRAGMENT) in place of string interpolation arguments to preserve injected syntax structure as much as possible
* add tests
@mxstbr
Copy link
Member

mxstbr commented Aug 31, 2017

@undeadcat this is so awesome, very happy to see you're working on support!

I've invited you to the organization on GitHub, that should make collaboration easier-let me know who else to invite.

@daedlock
Copy link
Member

daedlock commented Aug 31, 2017

@undeadcat that's a real game changer!!! thanks for the support. I will test it myself and release this ASAP! Thanks again.

@undeadcat
Copy link
Contributor Author

@mxstbr or @daedlock,
When you have time, please look over the changes to see that they make sense.
I'm not going to merge this (but thanks for giving me the rights anyway!), you should check that it works and doesn't break any scenarios you intended to support.

You can add @prigara (https://github.com/prigara)
She is our marketing manager, she will be able to help with documentation and how best to publish the plugin to the plugins repository

@mxstbr
Copy link
Member

mxstbr commented Aug 31, 2017

I don't know Kotlin or Java, so I'll defer to @daedlock for the review. Have added @prigara!

@oliverturner
Copy link

This is just awesome. Thanks so much @undeadcat and @daedlock!

@daedlock
Copy link
Member

daedlock commented Aug 31, 2017

@undeadcat Just tested the plugin on 2017.1. It works like charm and fixes all problems in my implementation! Although there are a few of points we need to reference, some of which where mentioned in the TODO section in the readme

  1. Add support for css tagged templates. We currently support styled.*, styled(), *.extends.

  2. Smart indent closing backtick when hitting enter while the cursor is at styled.div`|.

  3. When reformatting the the document using Action Pane > Reformat Code, the CSS blocks shall be formatted and the closing backticks should be placed on the same indent level as the opening one with indent applied from users code style prefs.

Example:

class X {
    constructor() {
      const Box = styled.div`
margin: 0;
padding: 0;
`;
    }
}

When reformatted using 4-space code style scheme for JS & LESS, it should be:

class X {
    constructor() {
        const Box = styled.div`
            margin: 0;
            padding: 0;
        `;
    }
}

Do you prefer adding the top points to this PR or shall I create separate issues for them?

@undeadcat
Copy link
Contributor Author

undeadcat commented Aug 31, 2017

@daedlock,

  1. Sorry, i'm not sure I understand what you're saying.
    Do you mean to say it doesn't work for you?
    It works for me.
    image

or that it works, but shouldn't be supported?
I was looking at API usage examples here : https://www.styled-components.com/docs/api and it includes css``.

  1. This isn't specific to styled-components. There is currently a limitation of the formatter in IDEA/WebStorm that injections that contain more than one part (e.g, interpolation arguments) cannot be formatted. You can see it using this js code:
//language=HTML
let withoutArguments = `
    <div>
        <div></div>
    </div>`


//language=HTML
let withoutArguments = `<div><div>${withArgument()}</div></div>`

At the moment it doesn't seem like something that can be addressed on the plugin side with the current IDE versions.
We have plans to improve this starting with 2017.3
You can follow this issue

@daedlock
Copy link
Member

Sorry, i'm not sure I understand what you're saying.
Do you mean to say it doesn't work for you?
It works for me.

Nevermind that! It works fine.

At the moment it doesn't seem like something that can be addressed on the plugin side with the current IDE version.
We have plans to improve this starting with 2017.3

Would be great to have that. Hopefully this would be already fixed in the next WebStorm release.

For now, I think the code looks great! Will add a release and update the README!

Thanks a lot @undeadcat for the amazing work!

@daedlock daedlock self-requested a review August 31, 2017 17:08
@daedlock daedlock merged commit 6aec9b5 into styled-components:master Aug 31, 2017
@prigara
Copy link
Collaborator

prigara commented Aug 31, 2017

Cool! Thank for merging. @daedlock I've also main some new screenshots, I will add them to the Readme later today.

@mxstbr
Copy link
Member

mxstbr commented Aug 31, 2017

Yessss, this is great!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants