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 bundled support for styles in template literals #3405

Merged
merged 1 commit into from
Jul 21, 2018
Merged

Conversation

gucong3000
Copy link
Member

Which issue, if any, is this issue related to?

#3264

Is there anything in the PR that needs further explanation?

e.g. "No, it's self explanatory."

@gucong3000 gucong3000 force-pushed the postcss-styled branch 2 times, most recently from 79de13a to 4aac942 Compare June 20, 2018 08:36
Copy link
Member

@alexander-akait alexander-akait left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why we remove postcss-markdown? We are using this for linting css in markdown

@gucong3000 gucong3000 force-pushed the postcss-styled branch 2 times, most recently from 9bf4a3f to 51af626 Compare June 20, 2018 10:53
@ntwb
Copy link
Member

ntwb commented Jun 20, 2018

@evilebottnawi Please read the discussion in #3264 that you yourself had given a 👍 to.

If you have a question on a PR also please add that as a question in a comment or make your PR review a comment, but please don't make these types of questions/comments as "Request Changes" as this is not the intended purpose of the "Request Changes" feature.

@alexander-akait
Copy link
Member

alexander-akait commented Jun 20, 2018

@ntwb i support css-in-js out of box, but how it is related to markdown? We should support markdown and css-in-js out of box 😕 And i use Request Changes, because css-in-js is not related to markdown.

@@ -12,7 +12,7 @@ module.exports = function(selector /*: string*/) /*: boolean*/ {
}

// SCSS placeholder selectors
if (selector.indexOf("%") === 0) {
if (selector[0] === "%") {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regression, i can use .my-class%foo { }

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

".my-class%foo".indexOf("%") === 0
// false

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

selector.startsWith("%") is much clearer in the intent... assuming that I'm reading the intent of that code correctly in the first place!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rollback of unrelated modifications in 48d4788

package.json Outdated
"postcss-media-query-parser": "^0.2.3",
"postcss-reporter": "^5.0.0",
"postcss-resolve-nested-selector": "^0.1.1",
"postcss-safe-parser": "^3.0.1",
"postcss-sass": "^0.3.0",
"postcss-scss": "^1.0.2",
"postcss-selector-parser": "^3.1.0",
"postcss-syntax": "^0.28.0",
"postcss-styled": "github:gucong3000/postcss-styled#literal_part",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think better use stable release for this

@alexander-akait
Copy link
Member

Also PR title is misleading 😕 because in package.json we still have postcss-markdown

@ntwb
Copy link
Member

ntwb commented Jun 20, 2018

Sorry @evilebottnawi, thanks for that clarification, I missed that myself 😞

@gucong3000 gucong3000 force-pushed the postcss-styled branch 3 times, most recently from 1097434 to 6d57272 Compare June 21, 2018 01:37
@gucong3000 gucong3000 changed the title [WIP] replace postcss-markdown with postcss-styled [WIP] Add build-in CSS-in-JS support Jun 21, 2018
@gucong3000 gucong3000 force-pushed the postcss-styled branch 4 times, most recently from bbcc249 to 48d4788 Compare June 21, 2018 10:13
@gucong3000 gucong3000 changed the title [WIP] Add build-in CSS-in-JS support Add build-in CSS-in-JS support Jun 29, 2018
@gucong3000
Copy link
Member Author

@evilebottnawi
All fixed.

@jeddy3 jeddy3 mentioned this pull request Jun 29, 2018
@jeddy3
Copy link
Member

jeddy3 commented Jun 29, 2018

@gucong3000 Am I right in thinking we also need to bundle postcss-jsx if we wish to support the following out-of-the-box?:

import React from "react";

const Component = () => (
  <div style={{top: 0}}></div>
);

@gucong3000
Copy link
Member Author

Am I right in thinking we also need to bundle postcss-jsx if we wish to support the following out-of-the-box?

Yes, right.

@jeddy3
Copy link
Member

jeddy3 commented Jul 2, 2018

Yes, right.

Thanks. Shall we add it in this PR?

@gucong3000
Copy link
Member Author

gucong3000 commented Jul 2, 2018

Shall we add it in this PR?

postcss-jsx currently dependencies some non-stable version of @babel/*.
Maybe we need to wait for the official release of babel@7 and then add postcss-jsx.

@jeddy3
Copy link
Member

jeddy3 commented Jul 2, 2018

Maybe we need to wait for the official release of babel@7 and then add postcss-jsx.

Any ideas when babel@7 is due out?

We could always include postcss-jsx and flag it as experimental (which is what we did with the other syntaxes like Less and Sass).

@gucong3000
Copy link
Member Author

We could always include postcss-jsx and flag it as experimental.

If there is no objection, I'd like to add postcss-jsx in the next PR

Copy link
Member

@jeddy3 jeddy3 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gucong3000 I've requested some minor changes... just setting syntax: .. on objects or moving tests into their own testRule blocks to bring them in line with our conventions.

Once these changes are in, I think we can merge this and add bundled support for styled components.

@@ -114,6 +114,10 @@ testRule(rule, {
{
code: "a { -WEBKIT-border-radius: 12px; -webkit-BORDER-radius: 10px; }",
message: messages.rejected("-webkit-BORDER-radius")
},
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we please pull this out into its own testRule block with syntax: styled, please?

We should place it at the end of the file, after the syntax: html testRule block.

@@ -79,6 +79,10 @@ testRule(rule, {
code:
"a { border-top-width: 1px; border-bottom-width: 1px; border-left-width: 1px; border-right-width: 1px; }",
message: messages.expected("border-width")
},
{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we please pull this out into its own testRule block with syntax: styled, please?

We should place it at the end of the file.

@@ -100,6 +100,10 @@ testRule(rule, {
"-webKIT-transition",
"-WEBKIT-transition-property"
)
},
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we please pull this out into its own testRule block with syntax: styled, please?

We should place it at the end of the file.

@@ -69,6 +69,10 @@ testRule(rule, {
{
code: "a { background: url(var(--image)); }",
description: "ignore variable"
},
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we please pull this out into its own testRule block with syntax: styled, please?

We should place it at the end of the file.

@@ -185,6 +185,9 @@ testRule(rule, {
},
{
code: 'a { background: url("") }'
},
{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we please pull this out into its own testRule block with syntax: styled, please?

We should place it at the end of the file.

@@ -112,6 +112,50 @@ testRule(rule, {
]
});

testRule(rule, {
ruleName,
config: [1],
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add syntax: styled here to be explicit, please?


testRule(rule, {
ruleName,
config: [20],
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add syntax: styled here please.

@@ -34,6 +34,12 @@ testRule(rule, {
{
code: ".foo { *width: 100px; }",
description: "ignore CSS hacks"
},
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we please pull this out into its own testRule block with syntax: styled, please?

We should place it at the end of the file.

@@ -132,6 +132,9 @@ testRule(rule, {
{
code: ".foo /for/ .bar {}",
description: "reference combinators"
},
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we please pull this out into its own testRule block with syntax: styled, please?

We should place it at the end of the file.

@gucong3000
Copy link
Member Author

I've requested some minor changes...

Done.

@jeddy3
Copy link
Member

jeddy3 commented Jul 21, 2018

@gucong3000 Thank you. Those tests are much easier to scan and consume now.

LGTM.

Has anyone got time to add a 2nd review to this? I'll endeavour to release tomorrow if we get this merged.

Copy link
Member

@jeddy3 jeddy3 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! This is an exciting new direction for stylelint.

Copy link
Member

@ntwb ntwb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:

@jeddy3 jeddy3 changed the title Add build-in CSS-in-JS support Add bundled support for styles in template literals Jul 21, 2018
@jeddy3 jeddy3 merged commit 9bb9d17 into master Jul 21, 2018
@jeddy3 jeddy3 deleted the postcss-styled branch July 21, 2018 10:14
@jeddy3
Copy link
Member

jeddy3 commented Jul 21, 2018

  • Added: bundled support for styles in template literals (#3405).

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

Successfully merging this pull request may close these issues.

None yet

6 participants