mui / material-ui

Material UI: Ready-to-use foundational React components, free forever. It includes Material UI, which implements Google's Material Design.
https://mui.com/material-ui/
MIT License
91.86k stars 31.57k forks source link

Support JSS extend plugin by default #9342

Closed tdkn closed 5 years ago

tdkn commented 6 years ago

Expected Behavior

.App-text-273 {
    color: #FF0000; // <--- Expected
    cursor: pointer;
    font-size: 50px;
    user-select: none;
}

Current Behavior

.App-text-273 {
    extend: ext; // <--- Invalid
    cursor: pointer;
    font-size: 50px;
    user-select: none;
}

Steps to Reproduce (for bugs)

import React, { Component } from 'react'
import ReactDOM, { render } from 'react-dom'
import PropTypes from 'prop-types'
import { withStyles } from 'material-ui/styles'

const styles = theme => ({
  ext: {
    color: '#FF0000',
  },
  text: {
    extend: 'ext',
    fontSize: '50px',
    cursor: 'pointer',
    userSelect: 'none'
  }
})

@withStyles(styles)
class App extends Component {
  static propTypes = {
    classes: PropTypes.object.isRequired
  }

  onClick(event) {
    const className = event.target.getAttribute('class')
    const style = getComputedStyle(event.target, null)
    const { fontSize, cursor, userSelect, color } = style

    console.log(`className: ${className}`)
    console.log({ fontSize, cursor, userSelect, color })
  }

  render() {
    const { classes } = this.props
    return (
      <p className={classes.text} onClick={this.onClick}>Foo Bar Baz</p>
    )
  }
}

render(<App />, document.getElementById('root'))

Edit 88ywmypq7l

Environment

Tech Version
Material-UI 1.0.0-beta.22
React 16.0.0
browser Chrome Version 62.0.3202.94 (Official Build) (64-bit)
mbrookes commented 6 years ago

@tdkn Are you loading the jss-extend plugin? https://github.com/cssinjs/react-jss#custom-setup

tdkn commented 6 years ago

@mbrookes Thank you👍 I fixed it as below. But, this documented in detail? :disappointed:

import React, { Component } from 'react'
import ReactDOM, { render } from 'react-dom'
import PropTypes from 'prop-types'
import { JssProvider, SheetsRegistry, jss } from 'react-jss'
import { withStyles } from 'material-ui/styles'

const withJSSProvider = Component => props => (
  <JssProvider registry={new SheetsRegistry()} jss={jss}>
    <Component {...props} />
  </JssProvider>
)

const styles = theme => ({
  ext: {
    color: '#FF0000',
  },
  text: {
    extend: 'ext',
    fontSize: '50px',
    cursor: 'pointer',
    userSelect: 'none'
  }
})

@withJSSProvider
@withStyles(styles)
class App extends Component {
  static propTypes = {
    classes: PropTypes.object.isRequired
  }

  onClick(event) {
    const className = event.target.getAttribute('class')
    const style = getComputedStyle(event.target, null)
    const { fontSize, cursor, userSelect, color } = style

    console.log(`className: ${className}`)
    console.log({ fontSize, cursor, userSelect, color })
  }

  render() {
    const { classes } = this.props
    return (
      <p className={classes.text} onClick={this.onClick}>Foo Bar Baz</p>
    )
  }
}

render(<App />, document.getElementById('root'))

Edit 8n5lkpmm8l

giapelle commented 6 years ago

But jss-extend is not part of the jss-preset-default? https://material-ui.com/customization/css-in-js/#jss https://github.com/cssinjs/jss-preset-default/blob/master/package.json#L50

mbrookes commented 6 years ago

@giapelle Good point. @kof?

oliviertassinari commented 6 years ago

@tdkn Thanks for the feedback. I'm adding the change into the breaking change part of the changelog.

kof commented 6 years ago

We need to mention how user can customize a JSS instance in multiple places, because through this optimization, we made many users run into this and similar issues (still not convinced it was worth it)

mbrookes commented 6 years ago

@kof I think you may be right: #9335

oliviertassinari commented 6 years ago

@kof I think that we need to see this as an experiment. My strategy was to wait and see. If people complain about, then we add the plugins they miss.

oallouch commented 6 years ago

for me, it's all about composes

kof commented 6 years ago

@kof I think that we need to see this as an experiment. My strategy was to wait and see. If people complain about, then we add the plugins they miss.

It is basically like waiting for the first user who complains about the absence of a plugin which he is using, you will never finish this experiment haha

oallouch commented 6 years ago

"Composes" looks like a good solution to solve the shared-stylesheet-issue"

damianobarbati commented 6 years ago

Doh! @oallouch same here: I was sure mui was using jss-preset-default but then I stumbled into this. I was trying to use compose or extend. @olegberman was this removal worth if for 2kb gzipped after all? 🤔

kof commented 6 years ago

@oliviertassinari told ya

oliviertassinari commented 6 years ago

We have updated the documentation to reflect this change.

Vincz commented 6 years ago

I ran into the same issue, trying to use the extend plugin. I tried to follow this : https://material-ui.com/guides/right-to-left/#3-jss-rtl but I'm using server side rendering and I was not able to make it works.

PS: Not sure it was worth it to remove these plugins for 2kb as these are really awesome features that everyone should use 😁

medihack commented 6 years ago

I second that, extend and compose are IMHO essential tools in JSS and not worth the 2kb.

oliviertassinari commented 6 years ago

The rendering runtime overhead needs to be quantified. It's not just about bundle size.

kof commented 6 years ago

It is still optional for the mui, so if you find that it increases overhead, you can keep using classNames library and spread operator, I think though it won't make much difference.

p00h commented 6 years ago

I was trying to enable jss-extend plugin the exact way the jss-rtl plugin should be enabled according to docs. The same code, but I've replaced all plugin-related calls to jss-extend one. But without success. The rules that should be extended actually are not affected. Could anyone have made it eventually?

nsaubi-swi commented 6 years ago

Damn, half last day spent on how to use jss-extend in material-ui, with this example found somewhere: cssLabel: { "&$cssFocused": { color: grey[500] } }, cssFocused: { backgroundColor: blue[500] }, where I though "&$cssFocused would be like extend: cssFocused. But it's not. So how to do it ?

kof commented 6 years ago

@nsaubi you are talking about nesting functionality, & is replaced by the containing rule selector, same like in sass.

jlissner commented 5 years ago

Any info on if jss-extend/compose will become standard in the future?

aemc commented 5 years ago

I hope extend makes it back since it helps making the jss code look cleaner imo.

gitsupersmecher commented 5 years ago

++ (1 )

2kb does not really matter. I think that establishing a good coding pattern with "extend and compose" is more important for the future. @oliviertassinari Could you please evaluate to add them back, and actually, to add full support for them?

oliviertassinari commented 5 years ago

@gitsupersmecher You can already have full support for "extend and compose" by adding the plugin to JSS with the JssProvider component. Most people can get along without. +2kb bundle increase is one side of the issue, runtime cost is the other.

kof commented 5 years ago

I think runtime cost is almost not noticeable for those 2. I also think its worth adding a few kb for this, since we made styling part of the public api and those are very popular things to do.

However instead of extend, one could use spread operator, which covers most cases and composition can be done inside of a component by concatenating a className.

dreamingblackcat commented 5 years ago

:+1: on re-adding back those two (extend and compose). They're definitely worth it for a mere 2KB IMO.

kof commented 5 years ago

another option is to really hide internal jss instance and make it very clear how to use own jss setup for styling.

fedeiglesias commented 5 years ago

Hi, im triying to add jss-extend in a nextjs project, (using the jss-rtl plugin example) without success. Somebody can make it work ?

aemc commented 5 years ago

^ would like to know as well

fedeiglesias commented 5 years ago

Here i post what i do. i dont know how to implement generateClassName because in the nextjs example is alredy used .

Taking as a base this example: https://github.com/mui-org/material-ui/tree/master/examples/nextjs

First install jss-extend npm i --save jss-extend

/pages/_app.js

import React from 'react';
import App, { Container } from 'next/app';
import { MuiThemeProvider } from '@material-ui/core/styles';
import CssBaseline from '@material-ui/core/CssBaseline';
import JssProvider from 'react-jss/lib/JssProvider';
import getPageContext from '../src/getPageContext';

/* --------------------------------------------------------------------------------
* ADD THE IMPORTS
*-----------------------------------------------------------------------------------*/
import { createGenerateClassName, jssPreset } from '@material-ui/core/styles';
import { create } from 'jss';
import jssExtend from 'jss-extend';

const jss = create({ plugins: [...jssPreset().plugins, JssProvider ()] });

class MyApp extends App {
  constructor(props) {
    super(props);
    this.pageContext = getPageContext();
  }

  pageContext = null;

  componentDidMount() {
    // Remove the server-side injected CSS.
    const jssStyles = document.querySelector('#jss-server-side');
    if (jssStyles && jssStyles.parentNode) {
      jssStyles.parentNode.removeChild(jssStyles);
    }
  }

  render() {
    const { Component, pageProps } = this.props;
    return (
      <Container>
        {/* Wrap every page in Jss and Theme providers 
             HERE ALSO ADD JSS TO PROVIDER
          */}
        <JssProvider
          jss={this.jss}
          registry={this.pageContext.sheetsRegistry}
          generateClassName={this.pageContext.generateClassName}
        >
          {/* MuiThemeProvider makes the theme available down the React
              tree thanks to React context. */}
          <MuiThemeProvider
            theme={this.pageContext.theme}
            sheetsManager={this.pageContext.sheetsManager}
          >
            {/* CssBaseline kickstart an elegant, consistent, and simple baseline to build upon. */}
            <CssBaseline />
            {/* Pass pageContext to the _document though the renderPage enhancer
                to render collected styles on server side. */}
            <Component pageContext={this.pageContext} {...pageProps} />
          </MuiThemeProvider>
        </JssProvider>
      </Container>
    );
  }
}

export default MyApp;
yidongw commented 5 years ago

Is there any plan for adding extend back?

yidongw commented 5 years ago

I'm following the guide on https://material-ui.com/guides/right-to-left/#3-jss-rtl, and changed the rtl to extend, but it doesn't work. Any insight on why?

ilyador commented 5 years ago

I am doing this and the plugin is not loading (v3.2.2) Any ideas?

import React from 'react'
import ReactDOM from 'react-dom'
import {
  MuiThemeProvider,
  createMuiTheme,
  createGenerateClassName,
  jssPreset
} from '@material-ui/core/styles'
import JssProvider from 'react-jss/lib/JssProvider'
import { create } from 'jss'
import jssExtend from 'jss-extend'
import CssBaseline from '@material-ui/core/CssBaseline'
import { UserProvider } from './context/User'
import App from './components/App'

const jss = create({ plugins: [...jssPreset().plugins, jssExtend()] })
const generateClassName = createGenerateClassName()

const theme = createMuiTheme({ ... })

const Main = () => (
  <JssProvider jss={jss} generateClassName={generateClassName}>
    <MuiThemeProvider theme={theme}>
      <CssBaseline/>
        <App/>
    </MuiThemeProvider>
  </JssProvider>
)
kof commented 5 years ago

You have to set up plugins in the right order, http://cssinjs.org/plugins?v=v9.8.7

You are using .a preset created for mui and adding jssExtend in the wrong order. Instead you could use a jss instance from react-jss that already has all presets applied: import {jss} from "react-jss" or import the default preset from the http://cssinjs.org/jss-preset-default?v=v4.5.0

rdsedmundo commented 5 years ago

Just run into this issue. Also believe it should have been in-place by default.

m-a-r-c-e-l-i-n-o commented 5 years ago

I would argue that it should also be in place by default on the basis that @oliviertassinari 's reasoning is likely not strong enough. In part, because if extend/compose are not encouraged, there's a good chance the codebase might end with additional bloat from unnecessary CSS — which could also lead to unnecessary processing. Maybe not as harmful as 2kb and maybe not as much processing as extend/compose would do, but code maintenance is also a factor here. In conclusion, my reasoning might not be that strong either, but in the presence of not so strong reasoning for the options at hand... I say give the crowd what they want! :D

AdamWhitehurst commented 5 years ago

I agree that extend and compose should be included by default. They are such fundamental functions and the reason I use JSS. I would love to see them re-added, as even small projects will likely use them and the custom install is very lengthy comparatively.

oliviertassinari commented 5 years ago

@AdamWhitehurst Could you upvote the issue? It would help us make a better tradeoff :).

I think that we have to consider the following weighted dimensions:

Eventually, we could run a Twitter poll for it 🤔.

kof commented 5 years ago

runtime cost if you have extend/compose plugins installed is almost 0, its a check if extend or composes property is defined, if not - it does nothing

AdamWhitehurst commented 5 years ago

Totally, I upvoted the first post, is that what you mean? I'm sorta new to all this, so bear with me, haha.

aemc commented 5 years ago

Take my upvote as well

AdamWhitehurst commented 5 years ago

Another consideration might be how performant workarounds are. Example from my code:

<Typography variant='h5' className={`${classes.sideLink} ${classes.leftSideLink}`} >About</Typography>

What would the cost of doing this workaround be?

gabrielliwerant commented 5 years ago

Any news on this issue for extend and compose? Like others, I also was unable to follow the docs to install separate plugins. I even copied the example but ran into issues with StylesProvider.

oliviertassinari commented 5 years ago

@gabrielliwerant It should be easy to add new plugins. If Material-UI fails at executing on that then we should focus on this problem.

I'm closing as we have already added the wait for upvotes label.

lookfirst commented 4 years ago

@gabrielliwerant

Latest is currently at: https://github.com/cssinjs/jss/tree/master/packages/jss-plugin-extend

npm i jss-plugin-extend
import {jssPreset, StylesProvider, ThemeProvider} from '@material-ui/styles';
import {create} from "jss";
import jssPluginSyntaxExtend from "jss-plugin-extend";

const jss = create({
    plugins: [...jssPreset().plugins, jssPluginSyntaxExtend()],
});

Then something like this seems to work well enough...

    render() {
        const {Component, pageProps} = this.props;

        return (
            <Container>
                <Head>
                    <title></title>
                </Head>
                <StylesProvider jss={jss}>
                    <ThemeProvider theme={theme}>
                            <CssBaseline/>
                            <Component {...pageProps} />
                    </ThemeProvider>
                </StylesProvider>
            </Container>
        );
    }
verbart commented 4 years ago

@gabrielliwerant

Latest is currently at: https://github.com/cssinjs/jss/tree/master/packages/jss-plugin-extend

npm i jss-plugin-extend
import {jssPreset, StylesProvider, ThemeProvider} from '@material-ui/styles';
import {create} from "jss";
import jssPluginSyntaxExtend from "jss-plugin-extend";

const jss = create({
  plugins: [...jssPreset().plugins, jssPluginSyntaxExtend()],
});

Then something like this seems to work well enough...

  render() {
      const {Component, pageProps} = this.props;

      return (
          <Container>
              <Head>
                  <title></title>
              </Head>
              <StylesProvider jss={jss}>
                  <ThemeProvider theme={theme}>
                          <CssBaseline/>
                          <Component {...pageProps} />
                  </ThemeProvider>
              </StylesProvider>
          </Container>
      );
  }

I tried this solution, but in this version only jss-plugin-extend works, all the built-in plugins (... jssPreset ().plugins) do not work, as if they were not there. material-ui v4.5, jss v10

verbart commented 4 years ago

This is the solution that works for me:

import jss from 'jss';
import preset from 'jss-preset-default';
import { StylesProvider } from '@material-ui/styles';

jss.setup(preset());
<StylesProvider jss={jss}>
  ...
</StylesProvider>
"@material-ui/styles": "^4.5.0",
"jss": "^10.0.0",
"jss-preset-default": "^10.0.0-alpha.27"
vlavella commented 4 years ago

Hi guys, it is possible to use MuiThemeProvider and use jss plugins as well? The documentation seems to be a little old, and I have a solution that stops working with extend and compose. I suppose that is because of some change of context. This is how my code looks like:

import { MuiThemeProvider, createMuiTheme } from '@material-ui/core/styles';
import { StylesProvider, jssPreset, createGenerateClassName } from '@material-ui/styles';
import { create } from 'jss';
import jssCompose from 'jss-plugin-compose';
import jssExtend from 'jss-plugin-extend';

const ThemeProvider = ({ children }) => (
  <StylesProvider jss={jss} generateClassName={generateClassName}>
    <MuiThemeProvider theme={theme}>
      <ThemeContext.Provider value={theme}>{children}</ThemeContext.Provider>
    </MuiThemeProvider>
  </StylesProvider>
);
oliviertassinari commented 4 years ago

Yes, it's possible, it's a requirement of the right-to-left support for the library.