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

How to work with composes? #195

Closed
kumarharsh opened this issue Nov 24, 2016 · 18 comments
Closed

How to work with composes? #195

kumarharsh opened this issue Nov 24, 2016 · 18 comments

Comments

@kumarharsh
Copy link

I think composes is a great idea on paper. But after giving it a go, I find there are some issues in using it in a real-world project.

One issue I've ran into is something like this:

  1. Say I keep all my animations in "css/shared/animations.css", like an animation called moving-stripes
  2. Now, in my Button.css, I write this:
.button {
  background-image: linear-gradient(to bottom right, white40 0%, white20 100%);
}
.busy { /* @TODO: this outputs a blank class */
  composes: moving-stripes from "css/shared/animations.css";
  background-image: repeating-linear-gradient(135deg, transparent 0%, white40 50%, transparent 100%);
}

But this .busy class gets the same specificity as .button, and gets overridden by .button's styles (depending on order - this happens for me).

To fix it, I try to do this:

.button.busy { /* @TODO: this outputs a blank class */
  composes: moving-stripes from "css/shared/animations.css";
  background-image: repeating-linear-gradient(135deg, transparent 0%, white40 50%, transparent 100%);
}

which I can't do - because composes doesn't work with two classes. So, now I'm stuck in a catch-22 situation, with no way out.

The only thing which works is this:

.busy { /* @TODO: this outputs a blank class */
  composes: moving-stripes from "css/shared/animations.css";
}
.spinner-wrapper.busy {
  background-image: repeating-linear-gradient(135deg, transparent 0%, white40 50%, transparent 100%);
}

Although this works, the solution has two drawbacks which I see:

  1. This outputs an empty .Button__busy___2QL1h rule.
  2. I feel that this is very prone to breakage as time goes on, and more complexity is introduced, etc.

Is there a better way this can be done?

@thien-do
Copy link

thien-do commented Nov 24, 2016

To simplify, let's say I have a button, which is black when normal, and red on busy. Also, the button text is always white in all cases. All styles should be done in compose. This is my way:

background.css

.red {
  background: red;
}
.black {
  background: black;
}

button.css

._main {
  /* shared styles */
  color: white;
}
.normal {
  composes: _main;
  composes: black from 'background.css';
}
.busy {
  composes: _main;
  composes: red from 'background.css';
}

button.js

<button className={busy ? styles.busy : styles.normal} />

The point is: don't depend on css specificity. Depend on class combination

@thien-do
Copy link

Also, redundant empty class is another issue (and story). We are fighting it, but I suggest to not fighting it here 😄 here let's discuss about specificity

@kumarharsh
Copy link
Author

Thanks for your input @dvkndn. I'll post a more concrete code tonight which illustrates what I'm trying to say.

@thien-do
Copy link

Thank you @kumarharsh . Also try to simplify your code :D for example, instead of gradient background, you can use more simple css like color or background

@TheSisb
Copy link

TheSisb commented May 23, 2017

For animations you don't need compose.
I put my animations in a shared file like you:

@keyframes pulsing {
  0% {
    transform: scale(1);
    opacity: 1;
  }

  50% {
    transform: scale(0.8);
    opacity: 0.3;
  }

  100% {
    transform: scale(1);
    opacity: 1;
  }
}

Then in your other CSS just use it

//Buttons.css
@import '../shared/animations.css';
.button {
  animation: pulsing 1.4s infinite;
}

Might be some differences due to webpack settings, but I'm pretty sure this should work out of the box if you use postcss-import

@GodGinrai
Copy link

I don't know why this was closed. It doesn't look like the issue was addressed. 2 Work-arounds were suggested, but there has been no explanation if the issue @TrySound had was intended behavior or if there is some baked-in way of dealing with this problem other than trying to avoid triggering the issue.

@kumarharsh
Copy link
Author

@GodGinrai there isn't really a better solution than what was suggested here I think.

@chasen-bettinger
Copy link

How exactly does the pathing work?

I am getting ENOENT: no such file or directory, open 'root\client\components\Component\Presentational\assets\styles\index.scss'

Which is expected actually. How would I change the root to point to assets? I don't want to have obnoxious strings like '../../../../'

I want to emulate what I am seeing here: https://github.com/css-modules/webpack-demo/blob/master/src/components/3-ClassComposition/StyleVariantA/StyleVariantA.css

@kumarharsh
Copy link
Author

Use webpack's resolve.modules config for changing resolution paths: https://webpack.js.org/configuration/resolve/#resolve-modules

@chasen-bettinger
Copy link

@kumarharsh

I am using that

const dirNode = 'node_modules';
const dirApp = path.join(__dirname, 'client');
const dirAssets = path.join(__dirname, 'assets');

  resolve: {
    modules: [dirNode, dirApp, dirAssets]
  },

What exactly needs to be changed?

@tf
Copy link

tf commented Aug 18, 2020

I would have expected rules to be ordered topologically based on import dependencies. That would ensure that rules always come after rules they compose and thus be able to override declarations.

Tools like rollup-plugin-postcss do not seem to guarantee that though (see egoist/rollup-plugin-postcss#26).

tf added a commit to tf/pageflow that referenced this issue Aug 18, 2020
CSS modules rules can not reliably override rules from composed rules
[1]. Use more specific selector to ensure menu button can override
padding of unstyled button.

PostCSS Rollup plugin generates duplicate rules when importing another
rule in multiple places. Enable minification to remove those
duplicates in extracted CSS.

REDMINE-17871

[1] see css-modules/css-modules#195
tf added a commit to tf/pageflow that referenced this issue Aug 18, 2020
CSS modules rules can not reliably override rules from composed rules
[1]. Use more specific selector to ensure menu button can override
padding of unstyled button.

PostCSS Rollup plugin generates duplicate rules when importing another
rule in multiple places [2]. Enable minification to remove those
duplicates in extracted CSS.

REDMINE-17871

[1] see css-modules/css-modules#195
[2] egoist/rollup-plugin-postcss#26
@thien-do
Copy link

Well it's been a while :)) after 4 years of using CSS Modules I think it's better to not use "compose" anywhere else. Like not at all.

It's better to do that at template level. It's more suitable than dealing in CSS. For example if you are using React then that's "component".

@tf
Copy link

tf commented Aug 19, 2020

Yeah, sorry for commenting on such an ancient issue and thanks for getting back. But wouldn't I end up with the same problem if I tried to apply the button and the busy class from the original example to the same element in a component? One should probably try to avoid overriding/conflicting rules like that in the first place, but there are just some cases where it can be very convenient and so far I always thought compose would be the way to do it.

@thien-do
Copy link

thien-do commented Aug 19, 2020

Taking the original example of Button and Busy, this is what I'm going to do now (which may conflict with what I said 4 years ago at the start of the thread 😄):

  • If the styling logic "When something is busy, it should have a background image as A" is reusable, then I will put it into a CSS module on its own (busy.module.css for example, or you may call it animation)
  • For the styling logic "A button should have a background image as B", I'll put it into the module "button.module.css". Nothing special until now, right?
  • Now, I, as the author, know that a Button may also be Busy, in which case the "background-image" will be conflicted. Therefore, as the author of the Button, I'm gonna split the "background-image" part into its own selector!

Ok time to show the code

button.module.css

.button {
  border-radius: 3px; /* non conflicting styles */
}

.non-busy {
  background-image: A; /* because of the busy */
}

busy.module.css

.busy {
  background-image: B
}

button.tsx

import styles from "./button.module.css";
import busyStyles from "../somewhere/busy.modules.css";

const buttonClass = [
  styles.button,
  props.isBusy ? busyStyles.busy : styles.nonBusy
].join(" ");

The thing about this approach is the trade-off to let Button knows about the implementation detail of Busy, but what we gain is attributes exclusive, which I believe is worth the trade-off.

Note that this means the author of Button knows about Busy, but Busy doesn't know about Button at all.

@tf
Copy link

tf commented Aug 19, 2020

Thanks for the detailed example. This approach surely works, but it still creates a subtle sort of coupling: As you say, as the author of Button I need to know about Busy. But even worse I need to know about the internals of busy. Whenever I change busy, I need to check Button (and all other places like Button) to see if I need to move more now conflicting styles from button to nonBusy.

Just to provide a bit more context, here's my original example: I wanted to create a reusable CSS module unstyledButton that can be composed whenever I want to style a button to look more like a link:

utils.css:

.unstyledButton {
  border: 0;
  padding: 0;
  background-color: transparent;
  text-align: initial;
}

In some cases I do want my button to have a padding, though:

.someButton {
  composes: unstyledButton from './utils';
  padding: 10px;
}

As I learned the hard way, this does not work. Following your approach I could try to decompose unstyledButton into unstyledButtonWithoutPadding etc. but I will need to do that for every property that I want to override or even for combinations of properties. That defeats the purpose of the reusable rule.

Probably, trying to reuse rules like this is simply the wrong approach. Instead of composing an unstyledButton class in a lot of places, I should probably introduce an UnstyledButton component. But then I need to add props to control things like padding or background color (maybe on a higher abstraction level than pixels and rgb colors which might be a win in itself). This is fine, but it feels like I am required to start building a UI component library for something that would otherwise been quite ok to handle just in CSS modules.

@thien-do
Copy link

But then I need to add props to control things like padding or background color (maybe on a higher abstraction level than pixels and rgb colors which might be a win in itself).

Sorry that I couldn't help more :D however I think this makes sense. At the end of the day, I feel these approaches are the compromises that I need. I mean.. adding props feels like boilerplate, but it's also good because a component should not be changed in anyway that it doesn't allow explicitly, even with CSS, right?

I think it's a balance between safeness and convenient. In my real projects, I do find myself sometimes doing "hack" like .something:not(.a-fake-class) just to have a better priority :D

@tf
Copy link

tf commented Aug 20, 2020

Sorry that I couldn't help more

Oh no, this was a very helpful and insightful discussion for me. Thanks!

At the end of the day, I feel these approaches are the compromises that I need.

Yeah, it probably all comes back to what you said originally that using compose (especially across different files) is probably something to avoid if possible.

@tf
Copy link

tf commented Aug 20, 2020

One more thing I realized is that (as described here in the context of Webpack's mini-css-extract-plugin) at least for asynchronously loaded chunks, the thing I originally wanted compose to do can pretty much not be done.

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

No branches or pull requests

7 participants