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

Add support for styled-components #289

Merged
merged 1 commit into from Dec 14, 2016
Merged

Conversation

AriTheElk
Copy link
Contributor

This PR adds support for styled-components. They're a pretty big deal right now and have a good chance of becoming a standard. I think it would be awesome if you considered merging this upstream 😄

You can view a preview of the implementation here: http://garetmckinley.github.io/styled-babel-sublime

@mxstbr
Copy link

mxstbr commented Oct 15, 2016

Syntax highlighting is a part of a great experience when usingstyled-components. It's non-intrusive, meaning users who do not use styled-components won't notice any difference – but those who do definitely will.

In fact, the same syntax highlighting support was also added to Atom's language-babel package and people have only loved it so far!

We're working really hard to make styled-components the default solution for react apps. We think we nailed the development experience, and now we're working on further tools to get a slimmer filesize and even better performance.


have a good chance of becoming a standard

See this tweet 👍

@zertosh
Copy link
Member

zertosh commented Oct 24, 2016

Cool. We can do this. A few things: (1) What's with all of the unrelated changes? (2) Docs? (3) Tests?

@AriTheElk
Copy link
Contributor Author

What unrelated changes are you referring to? The branch attached to this PR should only have changes related to integrating styled-components. I have been patching things in this branch for a week though, so it got a little cluttered up. If you'd prefer I can start back from upstream and patch in only the final changes as a single commit.

For docs, want me to add a new section to the README to talk about styled-components integration with a few examples?

For testing, do I just need to create a new test/syntax_test_styles with all the possible matches? Or is there a way these files are getting generated?

Thanks

@zertosh
Copy link
Member

zertosh commented Oct 24, 2016

What unrelated changes are you referring to?

  • The firstLineMatch removal.
  • Re-sorting the rules.
  • Removing the comments.

If you'd prefer I can start back from upstream and patch in only the final changes as a single commit.

Yes please :)

For docs, want me to add a new section to the README to talk about styled-components integration with a few examples?

That would be great!

For testing, do I just need to create a new test/syntax_test_styles with all the possible matches? Or is there a way these files are getting generated?

I don't know if you can auto-generate them. Just something that tests that the source.react.css scope is applied where you expect.

@AriTheElk
Copy link
Contributor Author

Ok, thanks! I appreciate your input and the consideration of this PR 👍 I'll get to these changes this afternoon.

@AriTheElk
Copy link
Contributor Author

I do have one question about this repo, what is the point of the JavaScript (Babel).tmLanguage file? In the README, it specifies that only ST3 is supported, which means that with the sublime-syntax file present, the tmLanguage file is ignored. Correct me if I'm wrong on this.

@zertosh
Copy link
Member

zertosh commented Oct 25, 2016

Correct me if I'm wrong on this.

You're right, but only if you're using the latest ST3 beta build - not all ST3's supported sublime-syntax. Very shortly (2 weeks maybe? I have an 8 hrs flight coming up to do this), I'm removing the sublime-syntax and only keeping the tmLanguage. The idea is to have one grammar that works in ST3, Atom and VSCode. The current tmLanguage sucks. However, TypeScript's just got a re-write, and it's perfect to adapt to Flow-typed JS. This why I'm asking you for a test - I don't use styled-components, so I want to be able to catch breakage when I make the switch.

@mxstbr
Copy link

mxstbr commented Oct 25, 2016

This why I'm asking you for a test - I don't use styled-components, so I want to be able to catch breakage when I make the switch.

That sounds very reasonable 👌

@AriTheElk
Copy link
Contributor Author

Ah, that makes sense. I'm working on the test file now, almost done. I already had implemented the support for styled-components in both the tmLanguage and sublime-syntax files, do you want me to include the changes in both those files until you actually remove the sublime-syntax from the repo?

It's no extra work for me either way since I had already written the code, just wanna make sure I include only the changes you want.

@zertosh
Copy link
Member

zertosh commented Oct 25, 2016

Yes please, include both. I suspect that most (if not all) users will use the sublime-syntax implementation, but it would be nice if I had something already in the tmLanguage that I can copy/paste 😄

@bhough
Copy link

bhough commented Dec 1, 2016

garetmckinley @zertosh anything I can help with to move this along?

@AriTheElk
Copy link
Contributor Author

Hmm, I see that the repo has been updated recently! Basically we just need to make a fresh fork of the master repo and then patch in some of my changes. The main changes are in the new Styled Components.tmLanguage file.

I don't have time today, but I can definitely get to this and have it wrapped up tomorrow.

@zertosh
Copy link
Member

zertosh commented Dec 1, 2016

@garetmckinley can you rebase it onto master and only do the tmLanguage?

@mxstbr
Copy link

mxstbr commented Dec 2, 2016

Amazing, can't wait to land this! 🎉

@AriTheElk
Copy link
Contributor Author

So I cleaned up this branch's history and got it ready for merging 😸

I added better language definitions to the Styled Components.tmLanguage file. It even supports using css-only sublime extensions inside the embedded CSS! Like the css color preview extension shown in the screenshot 😄

I also added fairly extensive tests for the Styled Components.tmLanguage file.

preview

Let me know if there's anything else you need me to do before we get this merged!

@mxstbr
Copy link

mxstbr commented Dec 3, 2016

That's amazing, looks really really good @garetmckinley!

@zertosh
Copy link
Member

zertosh commented Dec 6, 2016

@garetmckinley did you edit JavaScript (Babel).tmLanguage directly? Check out https://github.com/babel/babel-sublime#contributing.

@AriTheElk
Copy link
Contributor Author

Yup. That section confused me, as theres no YAML-tmKittens file. I thought maybe it was referring to the YAML-tmLanguage file, but looking at the repo that file hasnt been updated in nearly a year. Let me know what you want me to edit and ill get it changed.

At this point im just moving around regex haha so its no big deal.

@zertosh
Copy link
Member

zertosh commented Dec 6, 2016

yes, please update the YAML-tmLanguage file since that's what is used to generate the tmLanguage file.

@AriTheElk
Copy link
Contributor Author

You got it 👍

@AriTheElk
Copy link
Contributor Author

This PR should be ready to go @zertosh. The styled-components blocks were ported to the YAML file and I rebuilt the tmLanguage file using the updated template as well. I also got all 112 syntax tests passing as well 👍

@zertosh zertosh merged commit 2bcc72c into babel:master Dec 14, 2016
@zertosh
Copy link
Member

zertosh commented Dec 14, 2016

Thanks @garetmckinley! I need to get a few more changes in before I put out a release, so it'll be a few more days.

@AriTheElk
Copy link
Contributor Author

No worries, I'm glad we were able to get this merged! I apologize for my misunderstanding how to use the DevTools, this was my first time working on a syntax package. I feel like I learned a lot though and I really appreciate your patience and willingness to help ☺️

@AriTheElk
Copy link
Contributor Author

Yeah, just clone the repo to your sublime packages directory.

@ericbiewener
Copy link

Great job with this! I'm using this PR with my Sublime and noticed the following syntax highlighting issues:

  1. styled(...) syntax loses all highlighting
  2. transform: property isn't highlighted correctly

image

@AriTheElk
Copy link
Contributor Author

AriTheElk commented Jan 29, 2017

Thanks for reporting @ericbiewener! I've been pretty busy the past few weeks, but I have a little free time this afternoon, so I'm going to try and fix the issues that have been reported in this thread. Let me know if there's any other rules that don't work!

The CSS highlighting was based off of the original CSS language file from sublime, so I think it's a little dated in terms of modern css rules. I don't think things like flex are highlighted either, I may be wrong though; I don't use sublime as my primary editor.

@ericbiewener
Copy link

Well, since you're working on it, let me add one more highlighting request ;)

Although the CSS highlighting doesn't work with the styled(...) syntax, normal JS string interpolation highlighting does. It would be great to get this within the CSS-highlighted syntax. Compare the ${props => ... } syntax highlighting below to the version in Container in the first screenshot I posted above.

image

@AriTheElk
Copy link
Contributor Author

This is definitely something on my list, but I'm unsure currently how to go about doing that since the CSS is an embedded language file. I'll have to do some research on it, so no promises on that one yet 😉

@mxstbr
Copy link

mxstbr commented Jan 29, 2017

Yo dawg, I heard you like syntax highlighting so I added an embedded language in an embedded language file in an embedded language... 😉

const Box = styled.div`
  background: yellow;
  ${props => props.big && css`
    font-size: 2em;
    color: ${props => props.red ? 'red' : 'blue'}
  `}
`

@mxstbr
Copy link

mxstbr commented Feb 2, 2017

Wait @zertosh was this shipped?!

@declanelcocks
Copy link

Also noticed that it doesn't work in this case:

image

And the syntax highlighting for &:hover is not working in this case:

image

I assume the above is not working anyway as it normally looks like this:

image

@AriTheElk
Copy link
Contributor Author

If anyone else wants to take a stab at these fixes, please feel free to. My schedule hasn't allowed me to spend very much time on things like this 😕

@zertosh
Copy link
Member

zertosh commented Feb 3, 2017 via email

@mxstbr
Copy link

mxstbr commented Feb 4, 2017

Thanks @zertosh!

@zertosh
Copy link
Member

zertosh commented Feb 6, 2017

@garetmckinley I made a lot of fixes in 1955e69. Can you give it a quick look before I publish?

@mxstbr
Copy link

mxstbr commented Mar 2, 2017

screen shot 2017-03-02 at 12 11 44

@zertosh I just tried this on a big project, and found this code ^ being syntax highlighted incorrectly. (using the master branch of babel-sublime) Any ideas why?

Another instance:

screen shot 2017-03-02 at 12 12 46

@zertosh
Copy link
Member

zertosh commented Mar 2, 2017

@mxstbr can I get the code and the name of theme you're using?

@mxstbr
Copy link

mxstbr commented Mar 2, 2017

Code
export const Bubble = styled.p`
	display: inline-block;
	flex: 0 0 auto;
	padding: 8px 16px;
	vertical-align: middle;
	border-radius: 16px
	margin-top: 2px;
	font-size: 14px;
	max-width: 60%;
	line-height: 20px;

	&:first-of-type {
		margin-top: 0;
	}

	a {
  		text-decoration: underline;
  		word-wrap: break-word;
  		line-height: inherit;
  		word-break: break-all;
  	}
`;

export const BubbleGroup = styled.div`
	width: 100%;
	margin-top: 8px;

	&:first-of-type {
		margin-top: auto;
	}

	> p {

		background-color: ${props =>
  props.me ? props.theme.brand.default : props.theme.generic.default};
		background-image: ${props =>
  props.me
    ? Gradient(props.theme.brand.alt, props.theme.brand.default)
    : Gradient(props.theme.generic.alt, props.theme.generic.default)}
		color: ${props =>
  props.me ? props.theme.text.reverse : props.theme.text.default};
		float: ${props => props.me ? `right;` : `left;`}
		font-weight: ${props => props.me ? `500` : `400`};
		clear: both;

		&:not(:first-of-type) {
			${props =>
  props.me ? `border-top-right-radius: 4px` : `border-top-left-radius: 4px`};
		}

		&:not(:last-of-type) {
			${props =>
  props.me
    ? `border-bottom-right-radius: 4px`
    : `border-bottom-left-radius: 4px`};
		}

		&::selection {
		  background-color: ${props =>
  props.me ? props.theme.text.default : props.theme.brand.alt};
}
	}
`;

@zertosh Color scheme: SublimeLinter/base16-ocean.dark (SL).tmTheme, theme: Spacegray.sublime-theme, font: Operator Mono

@zertosh
Copy link
Member

zertosh commented Mar 2, 2017

@mxstbr it's working as expected.

The s-c section is matched by a CSS grammar that's very close to the one included by Sublime. So if you grab your s-c css code, put it in a new view, and set the grammar to CSS, that should approximate what the s-c code should look like with your theme – and your code looks like your screenshots under base16-ocean.

As far as the interpolations in BubbleGroup, that brokeness is a limitation of the grammars. The content of styled.div is matched as CSS, and CSS doesn't have ${} interpolations, which is why it looks broken.

@mxstbr
Copy link

mxstbr commented Mar 2, 2017

Ah that's fair. Other than that everything works perfectly!

@danreeves
Copy link

danreeves commented Mar 17, 2017

@zertosh does that mean it would be possible to add ${} interpolations and CSS grammar together as a new styled-components grammar so we could have it highlighting properly, or is that a terrible idea?

Edit: looking at the atom impl, I think that's exactly what they've done

Edit2: I've opened a WIP pull request 👉 #311

@mxstbr
Copy link

mxstbr commented Aug 31, 2017

Any news on this being published? It's been sitting there for a while now... 😢

@zertosh
Copy link
Member

zertosh commented Aug 31, 2017

This implementation is not so great. I made lots of fixes in 1955e69, but I'm afraid to release it because it's still not-so-great and I don't really want to "support" it (in the address issues kinda way - not that it shouldn't be in the grammar). From what I understand of how styled-components works, the existing grammar's handling of CSS leaves a lot to be desired. So I don't really know how to proceed :/

@mxstbr
Copy link

mxstbr commented Aug 31, 2017

I'm happy to make it its own plugin under the styled-components org with what we have right now? What do you think about that, then at least we have something 😉

@zertosh
Copy link
Member

zertosh commented Aug 31, 2017

@mxstbr not sure what you mean by that... are you saying that instead of - include: source.js.css it includes some other grammar?

@mxstbr
Copy link

mxstbr commented Aug 31, 2017

No I mean extracting that part and making it it's own plugin, or can Sublime not run two syntax highlighters side-by-side?

@zertosh
Copy link
Member

zertosh commented Aug 31, 2017

@mxstbr that's not how grammars work :/

@mxstbr
Copy link

mxstbr commented Aug 31, 2017

That sucks, I guess we just gotta fork it then and publish it as styled-babel-sublime or something?

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

9 participants