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

feat: support imported classes in selectors (postcss-icss) #562

Closed

Conversation

apexskier
Copy link

Still work in progress, opening for visibility and guidance. Currently works locally but due to dependency updates tests don't pass.

What kind of change does this PR introduce?

Adds support for importing css modules in selectors. I guess it's a new feature, since this wasn't supported, but I'd consider it more of a bug fix, since the feature is part of the spec.

Did you add tests for your changes?

  • add tests
  • fix broken tests

Summary

This add support for importing css selectors from other locally scoped css files. This feature has been requested a few places and, in my opinion, allows css modules to use one of the core features that makes css great: being able to style components contextually.

A quick example of usage (using bare icss syntax):

/* btn.css */
.btn { /* ... */ }

/* grouped-btn.css */
:import("./btn.css") {
    btn: btn;
}
.grouped-btn .btn {
    /* changing stuff, maybe removing a border radius on inner .btns */
}

Previously, this would have been compiled to something like:

/* btn.css */
.__btn-style__btn { /* ... */ }

/* grouped-btn.css */
.__grouped-btn-style__grouped_btn .__grouped-btn-style__btn { /* ... */ }

Note that the .btn class has been scoped locally to the grouped button styles.

Now, this compiles to:

/* btn.css */
.__btn-style__btn { /* ... */ }

/* grouped-btn.css */
.__grouped-btn-style__grouped_btn .__btn-style__btn { /* ... */ }

Does this PR introduce a breaking change?

Possibly, this updates dependencies, and the base icss-utils repo has had a major version bump. I've got to investigate why tests are failing to know for sure. I'll update if so.

The main changes are based on css-modules/icss-utils@19ffee9

Also, thanks to @michael-ciniawsky for some context here - see #561 (comment)

@jsf-clabot
Copy link

jsf-clabot commented Jun 23, 2017

CLA assistant check
All committers have signed the CLA.

Copy link
Member

@michael-ciniawsky michael-ciniawsky left a comment

Choose a reason for hiding this comment

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

@apexskier Let's maybe wait until @TrySound can take a look at it, since he wrote the new implemenation and can comment on the possibilty to land this before 'new-loader' becomes a thing. Otherwise we better focus on landing that 😛

"postcss-modules-local-by-default": "^1.0.1",
"postcss-modules-scope": "^1.0.0",
"postcss-modules-values": "^1.1.0",
"postcss-icss-composes": "^2.0.3",
Copy link
Member

Choose a reason for hiding this comment

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

postcss-icss-keyframes is missing, that was splitted into it's own plugin :) for @keyframes support

Maybe postcss-icss-import (@import) && postcss-icss-url (url()) aswell

Copy link
Author

Choose a reason for hiding this comment

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

postcss-icss-url seems to break a bunch of stuff for me on first testing. Might not end up being worthwhile with the new-loader branch being in progress. I'd still like to get css-modules/postcss-icss-selectors#119 merged, since I think it'll be required even with new-loader and the reorganization of icss loaders.

}),
icssComposes,
parserPlugin(parserOptions)
Copy link
Member

Choose a reason for hiding this comment

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

Since the contract logic changes with renaming the various postcss-modules-* to postcss-icss-* my gut feeling tells me, that this will require a few changes to that internal plugin (parserPlugin) aswell

Copy link
Author

Choose a reason for hiding this comment

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

I think I've covered a bit of that above, but yeah, I'm not sure about everything.

@michael-ciniawsky
Copy link
Member

As it is likely for this to not work out, I'm closing the PR for now. Thx :)

@joepie91
Copy link

It's been a year since this was closed, and it's entirely unclear what is supposed to replace it. What am I supposed to use when I need this functionality (that is part of the spec)?

@alexander-akait
Copy link
Member

@joepie91 in near future we release css-loader@2 what support ICSS

@joepie91
Copy link

joepie91 commented Aug 27, 2018

@evilebottnawi If I need support for this right now, is there already a release candidate or Git branch or something that I can use for the time being? Or does the code for supporting it not exist at all yet?

(I'd be happy to test it and provide feedback.)

@alexander-akait
Copy link
Member

@joepie91 #760

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