cssinjs / jss

JSS is an authoring tool for CSS which uses JavaScript as a host language.
https://cssinjs.org
MIT License
7.08k stars 399 forks source link

[jss-important] Create a plugin which makes every value important. #209

Closed kof closed 7 years ago

kof commented 8 years ago

To increase specificity and avoid external CSS can break your component, add !important flag to every property automatically.

avocadowastaken commented 8 years ago

Currently i'm working with css, inline styles and jss in one project, and making properties !important in jss becoming everyday job. So this plugin will save lot of time for me.

kof commented 8 years ago

Nice, so why don't you create it? If you want we can add it as an official plugin in jsstyles org.

kof commented 8 years ago

The need for css is not nice of course, I hope the problem exists because you are migrating from css to jss + inline styles?

avocadowastaken commented 8 years ago

@kof 👍 will start it asap.

you are migrating from css to jss + inline styles?

I need css just for css resets and some theme typography rules.

As a theme i use material-ui - where everything is written with inline styles and to override something with jss i need !importants

kof commented 8 years ago

cc @nathanmarks I think material-ui team is working on migration to jss.

In the meantime yes, !important is the only way to overwrite inline styles from the outside.

However the need to overwrite component styles from the outside is a sign of bad design and will result in broken encapsulation.

Only components should be able to define there own look and feel, which means that they need to accept styles/classes over props.

avocadowastaken commented 8 years ago

I think material-ui team is working on migration to jss.

That's the main reason why i started migration to jss - callemall/material-ui#4066 (and found it very cool)

However the need to overwrite component styles from the outside is a sign of bad design and will result in broken encapsulation.

I just moved rules from style prop to @useSheet, i think i didn't broke anything if component already allowed me to override it's own styles (just colors and sizes)

But I get your point, i hate using !important in css (sass, less), but desperate times call for desperate measures (and i am just standing here and waiting for complete migration of material-ui)

nathanmarks commented 8 years ago

@umidbekkarimov

If you are using material-ui, I would recommend trying to wait until 0.16.0 as we are going to be implementing a style system that will make your life a lot easier. Theming will also be receiving some changes as a result -- so don't invest too much time in a workaround setup right now.

One of the driving factors behind the change is, as @kof pointed out, design flaws in the current setup with regards to styling. (Trust me, I understand your pain 👍 )

Ideally, in 0.16.0, the use of a better styling solution and simplification/improvement of overly complex components will mean that overriding from the outside should not be necessary. Especially not with !important for the sole purpose of overriding!

kof commented 8 years ago

@nathanmarks this is good news. Theming is the most underestimated topic in react components. The entire ecosystem of components didn't agree on one way to pass styles, which results in huge hassle everywhere.

This is why I started with theme-standard which tries to consider all possible use cases.

The idea is to have a theme object which can be used for theming without any additional tools. This object can style components using inline styles, classes and can contain options. Also recently I have added another property themes which allows us to nest themes. This is needed when you want to pass a theme for some inner component(s) which are used by the public component. This way we can use HOC's and lots of separate inner components and still be able to reach them from the theme.

Signature is basically this:

 {classes: {...}, styles: {...}, themes: {...}, someOption: 1}
nathanmarks commented 8 years ago

@kof

In your opinion, what are the advantages of having a theme standard to define how the application structures the global theme variables/styles? I think establishing a reliable method to use theme variables/styles within a modular component driven design is more important.

In terms of theme properties... theme.typography or theme.type? theme.colors or theme.palette? People are very opinionated here. Especially the designers and UX architects who write styleguides and design specifications 😉 . It's nice to be able to refer to parts of your theme from your code in an idiomatic fashion.

My point is that regardless of whether someone prefers colors or palette, implementing the use of that theme object is the difficult challenge IMO.

A big design challenge here is presented by the fact that components are modular, while themes (at least the specific properties that define it) are global by definition.

Here we also see a split in how one would use a theme w/ classes using less/sass (such as elemental-ui) versus how one would style components using JS styles.

To keep components theme-able and tight nit, an ideal scenario (IMO) if one is using JS styles is to have a crystal clear stylesheet definition in the component file:

const styleSheet = createStyleSheet('button', (theme) => {
  const { palette, shadows } = theme;

  return {
    base: {
      display: 'inline-flex',
      alignItems: 'center',
      justifyContent: 'center',
      height: 30,
      minWidth: 88,
      color: palette.accent.A100,
    },
    raised: {
      backgroundColor: palette.accent.A100,
      color: palette.accent.contrastText,
      boxShadow: shadows[2]
    }
  }
});

Another slightly tricky part here is making sure the functionality works properly not just in the browser, but in node too where concurrency will cause problems if constants like styleSheet are used to read the classes (this is one of the reasons why I currently have a thin abstraction over the Jss.StyleSheet object in my testing).

Rather than using props, I have used context to pass the necessary object down the tree. This is how we currently handle our theme object in material-ui using a ThemeProvider component.

Consider this quote from the React documentation:

The best use cases for context are for implicitly passing down the logged-in user, the current language, or theme information. All of these might otherwise be true globals, but context lets you scope them to a single React subtree.

In context I'm passing my "styleManager" (holds the theme too), which is responsible for passing the theme object into the style create callbacks passed into my createStyleSheet method, tracking component instance counts, and detaching/attaching sheets from the DOM. It also handles dynamic style updates when using webpack's hot module reload plugin with react-hot-loader. Lastly, it handles collecting and passing back the css for server side rendering, then reconciling with the associated <style> node on the client.

I currently have a little bit of boilerplate in my test component to support all of this:

  componentWillMount() {
    this.context.styleManager.attach(styleSheet);
  }

  componentWillUnmount() {
    this.context.styleManager.detach(styleSheet);
  }

and in render():

const classes = this.context.styleManager.getClasses(styleSheet);

In render() when getClasses(styleSheet) is called, if the module has been hot reloaded by webpack my styleSheet object will no longer match the reference that the styleManager has been using to track it. This means that the styleManager can detect this, find the style sheet with the matching name property and swap the JSS sheets out using the new callback.

Sorry for the long post -- I've been doing a lot of tinkering trying to figure out how to make this work for us and check all the boxes 😄

kof commented 8 years ago

In your opinion, what are the advantages of having a theme standard to define how the application structures the global theme variables/styles

  1. being able as a user to create a theme for components from npm in more or less the same way
  2. make maintainers think of theming and provide them with common ground for themification of their components. A place to learn about theming.

I think establishing a reliable method to use theme variables/styles within a modular component driven design is more important.

  1. User of a component doesn't care about how they internally apply the theme.
  2. There are lots of ways. One tool can't give the desired flexibility. Thats why I want to agree on a format first of all, then I want to provide separate optional tools, which will make the actual application of a theme for the publisher easy.

In terms of theme properties... theme.typography or theme.type? theme.colors or theme.palette

That's why I don't have anything like that in the standard, but flexible options, one can use any of these names.

implementing the use of that theme object is the difficult challenge

I want to provide more examples and some tools in the future, but I want those tools and examples come from real world projects. Thats why I think it is important to implement them first in material ui well and then extract those parts in separate projects and move them to theme-standard organization.

Also I think most of the time it will need more examples, there is no need for much libraries hier. I could imagine 2 libs so far:

  1. Theme Validation lib which will validate the theme structure according spec and provide to the user meaningful error messages to make it easier create a theme.
  2. Component or function which will accept a theme from pros as well as from context and do it right.

A big design challenge here is presented by the fact that components are modular, while themes (at least the specific properties that define it) are global by definition.

Modular vs. global? I think your wording is not quite correct. But I assume you mean that components can appear in any part of the render tree, but theme is defined once globally.

Rather than using props, I have used context to pass the necessary object down the tree. This is how we currently handle our theme object in material-ui using a ThemeProvider component.

I think it is important to have both options for theming: props and context. Context makes a global theme application easy. Props allow to pass a different theme to a specific component, which I was told is an important feature. I myself would not use context because I don't mind to create a themed component for every component I use, however I realize there are lots of people that don't want to make additional function call to start using a component.

I think something generic as 'styleManager' on context object is an anti-pattern. You can generate a namespace randomly and share this name over a constant and access this way you theme manager from context.

This is what I would make separate lib, which accepts theme from props and context, do.

I currently have a little bit of boilerplate in my test component to support all of this:

This boilerplate is exactly the reason for react-jss. This can be implemented without HOC too.

Sorry for the long post -- I've been doing a lot of tinkering trying to figure out how to make this work for us and check all the boxes 😄

It's dangerous to write long posts, you might get an even longer answer)

nathanmarks commented 8 years ago
  1. being able as a user to create a theme for components from npm in more or less the same way

I like the concept -- but it seems as though it would enforce rather rigid restrictions on components that want to fit seamlessly into the system, resulting in different themes that still feel rather "samey samey". This works within a single UI framework (think bootstrap themes), but I'm not sure if it will scale properly with the wide variety of designs out there, especially due to any specific DOM structures required to achieve a certain look/feel. Any sort of ecosystem might end up getting fragmented very quickly.

I may be completely wrong here, this is speculation based on my own experiences.

  1. make maintainers think of theming and provide them with common ground for themification of their components. A place to learn about theming.

Completely agree that there needs to be more of a common ground with regards to theming components 👍 . I don't necessarily think that it means that all themes/components need to be interoperable though.

That's why I don't have anything like that in the standard, but flexible options, one can use any of these names.

How would this fit into the idea presented above? (being able to pull standard-compliant components from npm and plug into them).

I want to provide more examples and some tools in the future, but I want those tools and examples come from real world projects.

Makes total sense, the best solutions usually evolve that way.

Modular vs. global? I think your wording is not quite correct. But I assume you mean that components can appear in any part of the render tree, but theme is defined once globally.

That was badly written on my part. I'm not even sure what I was trying to say now 😄

I think it is important to have both options for theming: props and context. Context makes a global theme application easy. Props allow to pass a different theme to a specific component, which I was told is an important feature. I myself would not use context because I don't mind to create a themed component for every component I use, however I realize there are lots of people that don't want to make additional function call to start using a component.

👍 100%

I think something generic as 'styleManager' on context object is an anti-pattern. You can generate a namespace randomly and share this name over a constant and access this way you theme manager from context.

This is what I would make separate lib, which accepts theme from props and context, do.

Putting styleManager on context was not my plan, initially it was an imported module. It ended up happening when I was solving the problem of concurrency in node. What I described above (creating a singleton object from a factory and passing it down through context) was my first pass. I would love to find a better solution for this.

Just to clarify, are you suggesting:

  1. my styleManager should be a separate lib
  2. Should not get not put on context
  3. styleManager encapsulates requests by creating an object internally, and returning a constant that can be used to reference that instance.

Something doesn't quite sit right with me about lugging it around on context, so I'm all ears here! 👂 👂

edit: just to clarify the above statement, I try to avoid context usually... so even use cases where it appears to fit make me question using it. Which is probably a good thing!

This boilerplate is exactly the reason for react-jss.

I actually did a bit of playing around with react-jss, but due to a couple of existing limitations I went off to experiment (can't remember which specific ones right now -- perhaps it was class names or server side rendering features).

In addition to HoCs/Child Components, I think it's also valuable to make it as easy as possible for users to integrate with a custom HoC etc that they already use for related purposes by exposing the necessary functions (or a quick guide) to help users follow a recommended design pattern.

One of my attractions to jss was the simple, framework agnostic approach and the lack of invasiveness necessary just to get started, with the potential to integrate however we want (our own HoC, a provided one, child component, or even just basic functions in components). One library I looked at required using a HoC that hijacked the component render by doing const output = super.render() (or something along those lines) -- something that turned me off quite a bit.

kof commented 8 years ago

I don't necessarily think that it means that all themes/components need to be interoperable though

I am even pretty sure the themes can't be interoperable ... it's just more of a common ground and making people think of a theme first when building a component.

How would this fit into the idea presented above? (being able to pull standard-compliant components from npm and plug into them)

Certainly one would need to create a theme for that installed component. We will be already lucky if we can do that properly at all.

Just to clarify, are you suggesting:

I suggest:

something that turned me off quite a bit.

Totally understand that.

nathanmarks commented 8 years ago
  • Put the theme on context, but generate the namespace
  • Use a separate module to grab the theme from context or props

Is the idea to have this separate module create "manager" objects internally and return a constant for the theme to hold? And then pass this back to said module upon attaching the stylesheet in components?

I think something generic as 'styleManager' on context object is an anti-pattern.

I want to revisit this one more time to make sure I understand the implications of such a design. In your opinion, what makes this an anti-pattern vs say, react-redux putting the redux store on context with the provider component?

Creating it via a factory, giving it knowledge of the theme being used and passing it down context seemed like a sensible approach to me to cleanly solve concurrency issues in a node environment.

I haven't tried the alternate approach that we've been discussing, but in some ways it feels as though another level of complexity is being added (the namespace system).

kof commented 8 years ago

Is the idea to have this separate module create "manager" objects internally and return a constant for the theme to hold? And then pass this back to said module upon attaching the stylesheet in components?

It is hard to talk about things of this complexity without code, so here I tried to write some pseudo code:

All public interfaces

// react-theme-standard.js
export {
  Theme,
  StylesManager
}

A very simple "namespace system")

// namespace.js
export default Math.random()

Internally Theme does something like this:

// Theme.js
import namespace from './namespace'

export default class Theme extends Component {
  constructor(props, context) { 
    super(props, context) 
    context[namespace] = props.src
  }
  ...
}

Wrapping App with Theme:

// ThemedApp.js
import {Theme} from 'react-theme-standard'

const themes = {
  Button: {
   // jss styles
   styles: {
     button: {
       color: 'green'
     }
   } 
}

<Theme src={themes}>
  <App />
</Theme>
// StylesManager.js
import namespace from './namespace'

export default StylesManager {
  constructor(name, props, context) {
    this.name = name
    this.props = props
    this.context = context
    this.classes = {}
  }

  setSystemStyles(styles) {
    this.systemStyles = styles
    return this
  }

  getTheme() {
    let {theme} = this.props
    if (!theme) {
      theme = this.context[namespace][this.name]
    }
    return theme
  }

  // Alternative to HOC.
  // Alternatively we can render a component which will ensure attaching and detaching, so that we don't hook reacts component.
  spy(element) {
    this.element = element
    this.originalComponentWillMount =  element.componentWillMount
    this.originalComponentWillUnmount =  element.componentWillUnmount
    element.componentWillMount = ::this.componentWillMount
    element.componentWillUnmount = ::this.componentWillUnmount
    return this
  }

  componentWillMount() {
    this.originalComponentWillMount.call(this.element)

    // TODO Add global caching for all instances ...
    if (!this.sheet) {
      const theme = this.getTheme()
      const styles = merge(this.systemStyles, theme.styles)
      this.sheet = jss.createStyleSheet(styles)
      this.classes = this.sheet.classes
    }

    // TODO Only if needed, add ref counting.
    this.sheet.attach()
  }

  componentWillUnmount() {
    this.originalComponentWillUnmount.call(this.element)
    // TODO Only if needed, add ref counting.
    this.sheet.detach()
  }
}

Using theme inside of a component without HOC:

// Button.js
import {StylesManager} from 'react-theme-standard'

export default class Button extends Component {
  constructor(props, context) {
    super(props, context)
    this.stylesManager = new StylesManager('Button', props, context)
      .setSystemStyles(systemStyles)
      .spy(this)
  }

  render() {
    return <button className={this.stylesManager.classes.button}>Button</button>
  }
}

In your opinion, what makes this an anti-pattern

I don't care what redux does, context is a global objects for all components, putting stuff there is exactly the same as putting it on window object. If you we do that, we need to make sure it can't conflict.

nathanmarks commented 8 years ago

Can you quickly outline where jss fits into the equation in your pseudo code above (where the actual StyleSheet objects are created)?

A very simple "namespace system")

There was a big misunderstanding about this 😂 Your pseudo code cleared it up.

I don't care what redux does, context is a global objects for all components, putting stuff there is exactly the same as putting it on window object. If you we do that, we need to make sure it can't conflict.

Why do we need a math.random() for making sure it won't conflict? Seems more sensible just to pick a good property name for your library or application.

For example, we currently use context.muiTheme for our theme object. Who on earth is using muiTheme in context in their application? Especially if they've decided to use material-ui already. We've never once had someone report an issue with a naming conflict on context due to this.

kof commented 8 years ago

Can you quickly outline where jss fits into the equation in your pseudo code above (where the actual StyleSheet objects are created)?

see StylesManager.componentWillMount

Why do we need a math.random() for making sure it won't conflict? Seems more sensible just to pick a good property name for your library or application.

Because this works only until somebody decides to use the same name for some weird reason. Lets not skip what we have learned about global objects.

Who on earth is using muiTheme in context in their application?

Who on earth are we to judge about it? Somebody does for sure ... it is always not a problem until it is a problem.

We've never once had someone report an issue with a naming conflict on context due to this

This means nothing. I just means that this guys didn't create an issue.

It is also not just a practical matter, you can say that this issue might happen to 0.1% of users and we can ignore them. But the problem is we are making an example. Imagine every module starts doing this.

This is exactly the reasoning why javascript has the global object. At the point of time it has been created it was fine for 99%. Don't you think this is not smart to rely on this logic?

nathanmarks commented 8 years ago

@kof

I think that using the window object is far more dangerous than using context in a component tree. Yes, both represent a pool of globals but context has a much more focussed purpose IMO (including some strict guidelines/recommendations from the FB team). window is just this bucket that everyone decides to chuck stuff in. And it is environment specific, it doesn't even exist in node (not the same object at least... global) whereas context is a feature of React.

In your componentWillMount, where does the jss instance come from?

To be compatible with server side rendering, we need to be able to have an object that has proper request encapsulation and lets us retrieve all of the stylesheet CSS output to render in the <head> of the document.

This is why I had sent my styleManager down the tree -- so each render of the application has it's own "manager" that gets created (with a new jss instance) that does not have a potential to spill over any sort of module state into other requests: https://github.com/nathanmarks/react-jsstyles-theme-experiment/blob/master/src/styles/styleManager.js

I am creating my styleManager and passing it into my ThemeProvider for the render. That way, when responding to the server request I can call styleManager.getSheets() after ReactDOM.renderToString() and collect all of the styles from that jss instance to render in the document head.

kof commented 8 years ago

In your componentWillMount, where does the jss instance come from

It should be a material-ui own jss instance.

I am creating my styleManager and passing it into my ThemeProvider for the render.

So this way you generate all css for every request, right? I assume you have somewhere dynamic options which might change sheets depending on request ... In this case yes you need to do something like that.

kof commented 8 years ago

@nathanmarks any news on your side? Have you seen the new id generation?

0xMarkian commented 8 years ago

@kof So what about the jss-important plugin?

kof commented 8 years ago

Haha, pretty much offtopic discussion here. I didn't create the plugin yet, if you want to do that, I can assist.

avocadowastaken commented 8 years ago

Here what I use as jss-important plugin

ilan-schemoul commented 7 years ago

Still interested by that, first day with this lib though.

kof commented 7 years ago

Somebody feel free to create such a plugin, it won't bee me as I don't need it.

rob2d commented 7 years ago

An alternative to a plugin here would also be possible to just have a wrapper function for a styleSheet object to generate each rule/function value evaluation as ${ruleName} !important, or to create an in between HOC for injectSheet and use that in your project.

(I'm not sure if anyone got around to it and don't want to be captain obvious, but since we can create styleSheets from essentially just a normal JS object, I just wanted to point this out as it is a simple pattern and one of the nicer things about JSS).

kof commented 7 years ago

@rob2d that would be a bad idea, because there are many syntax cases like nesting you would need to handle manually, where using plugins API you can focus on a simpler prop/value syntax.

rob2d commented 7 years ago

@kof I see. And I guess the API could also change or have additions in this regard, so this makes sense. As a small-scale solution though (and to play devil's advocate a bit), we could simply have that wrapper check for the leaf node of things being evaluated? There's probably flaws with this idea; but I am obviously not so familiar with the intricacies beyond simply declaring media/pseudoclasses and function values yet and curious about it.

Thanks for the input 😄

rdsedmundo commented 5 years ago

I was thinking that this would be very handy as well, searched and found this issue.

But the idea that I had on my mind was slightly different. I don't want to apply it for everything if it's not necessary – and even though I could have this important: false configuration that you suggested, I still don't want to bother in disabling it all the places it isn't necessary. What I was thinking was that adding a wrapper important key would rewrite all the properties inside it with the !important flag. i.e those examples below would be equivalent:

const styles = {
  div: {
     fontSize: 10,
     marginBottom: 15,
     paddingLeft: 15,
     important: {
       marginTop: 10,
       paddingRight: 10,
     },
   }
}
const styles = {
  div: {
     fontSize: 10,
     marginBottom: 15,
     paddingLeft: 15,
     marginTop: '10px !important',
     paddingRight: '10px !important',
   }
}

How difficult you believe the implementation of that would be, @kof?

kof commented 5 years ago

@rdsedmundo its not hard, but I wouldn't make it into the default preset. It seems people don't want to learn new syntaxes, so whatever we do syntax wise ends up either not being used or used in a wrong way. We should try fixing stuff without introducing new syntaxes as much as possible. With JSS there should be no need at all to use important.

The original title of this issue is about making all properties important. The use case for this is when e.g. writing a widget make sure external CSS doesn't interfere.

rdsedmundo commented 5 years ago

I understand. That's my use case as well. I don't like using !important often and yes, it's not even necessary most of the time with JSS.

The problem starts once you include components from 3rd party libraries like Material UI, Semantic UI, etc – some of them have support for overriding the styles via properties, but it's not the case always (like the styles got attached to the root container instead of the one you wanted to override, and your CSS doesn't overwrite unless !important is used`). That's why I wasn't wanting to overwrite everything, just some properties. And definitely, agree that it shouldn't be part of the default preset.

I may try to work on this one in the future, thanks for the information.

kof commented 5 years ago

If you don't share jss instance with mui, you can use insertionPoint and make sure your sheets are rendered after mui sheets and so having a higher source order specificity.

On Tue, Dec 4, 2018, 20:13 Edmundo Rodrigues notifications@github.com wrote:

I understand. That's my use case as well. I don't like using !important often and yes, it's not even necessary most of the time with JSS.

The problem starts once you include components from 3rd party libraries like Material UI, Semantic UI, etc – some of them have support for overriding the styles via properties, but it's not the case always (like the styles got attached to the root container instead of the one you wanted to override, and your CSS doesn't overwrite unless !important is used`). That's why I wasn't wanting to overwrite everything, just some properties. And definitely, agree that it shouldn't be part of the default preset.

I may try to work on this one in the future, thanks for the information.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/cssinjs/jss/issues/209#issuecomment-444221036, or mute the thread https://github.com/notifications/unsubscribe-auth/AADOWJykhfYW2FjotQY9zmfsBmLaWm9Vks5u1slkgaJpZM4IHlrz .

JoeDuncko commented 5 years ago

I'm working on an embeddable widget with JSS and having a plugin like this would save me a ton of headache. Did anyone ever build this? Otherwise, I'm going to try @rob2d 's HOC suggestion and go from there.

JoeDuncko commented 5 years ago

I ended up building a JSS plugin that just makes every CSS rule value !important. Would there be any interest if I open sourced it? It's pretty short.

kof commented 5 years ago

Btw have you tried this one? https://github.com/iamstarkov/jss-increase-specificity

JoeDuncko commented 5 years ago

Nope, but that might actually fix some of the other cases we are running into that !important isn't fixing. I'll check it out.

JoeDuncko commented 5 years ago

In case anyone is interested, I just open sourced our JSS plugin that makes every value !important. Feel free to check it out at https://github.com/JoeDuncko/jss-plugin-all-important .

There are some docs that explain how we used it in conjunction with other JSS plugins to protect the styles of the widget we built.