React-sortable-tree: [Discussion] Future of Styling/CSS and Theming

[brendanmoore]

Firstly I would like to say I love this library my use case(s) have been really good. While using it I personally have come across issues with how to style and add some subtle changes to behaviour (such as variable height nodes). From looking through the issues I am not the only one.

Currently the core CSS is consumed internally by the source code as a css-module (compiled from scss files), then when webpack bundles it, the classnames are prefixed with rst__. There is "a lot of magic" going on here and a lot of the styling issues in this repo would be avoided if it wasn't so magic.

It wouldn't be much work (say for a version 2) to stop using css-modules hardcode the "core" classnames in the jsx as strings such as ReactSortableTree__node, ReactSortableTree__rowand switch the webpack config to use ExtractTextPlugin to co-locate the css instead. That way new users of this library can easily see which components use what classname from reading the source, at the same time this would prevent the need to distribute style-loader logic making the bundle size smaller and more performant. The only difference to the end user is that they must import the css themselves but this is much more common (like react-virtualized, react-select, react-dates etc)

Proposal: (Repo source):

tree-node.js

// Currently
import React from "react";
import styles from "./tree-node.scss";

class TreeNode extends Component {
    /*...*/
    render() {
         /*...*/
        // styles.node hides the final classname from the user,
        // for devs who might not know webpack/css-modules
        // this could cause confusion 
        return (<div className={styles.node} {/*...*/} />);
    }
}

// Becomes
import React from "react"
import "./tree-node.scss"

class TreeNode extends Component {
    /*...*/
    render() {
         /*...*/
        // `"ReactSortableTree__node"` simpler to understand and override.
        return (<div className="ReactSortableTree__node" {/*...*/} />);
    }
}

tree-node.scss

/* Currrently */
.node {
  min-width: 100%;
  white-space: nowrap;
  position: relative;
  text-align: left;
}

/* Becomes */
.ReactSortableTree__node {
  min-width: 100%;
  white-space: nowrap;
  position: relative;
  text-align: left;
}

Proposal: (Usage):

import React, { Component } from 'react';
import SortableTree from 'react-sortable-tree';
// Now we just need to add the css into our project...
import 'react-sortable-tree/style.css';

export default class Tree extends Component {
  constructor(props) {
    super(props);

    this.state = {
      treeData: [{ title: 'Chicken', children: [ { title: 'Egg' } ] }],
    };
  }

  render() {
    return (
      <div style={{ height: 400 }}>
        <SortableTree
          treeData={this.state.treeData}
          onChange={treeData => this.setState({ treeData })}
        />
      </div>
    );
  }
}

Bonus: (Usage):

import React, { Component } from 'react';
import SortableTree from 'react-sortable-tree';
// Now we just need to add the css into our project...
import 'react-sortable-tree/style.css';
// Importing here will cascade as expected, because no-more style-loader! :)
import './my-react-sortable-tree-override-styles.css';

/*...*/

I am keen to contribute towards this change if you also think it is a good idea. What do you think?

[fritz-c]

Moving the css handling away from style-loader and into a css file you would import is an issue I’ve been mulling over for a long time now.

On the one hand, server side rendering becomes a lot more easy to handle, and overriding the styles also becomes easier, as you have demonstrated.
On the other hand, the one drawback that I’ve been lingering on is the fact that importing a css file yourself can be an extra hurdle for someone just starting to use the library. At least it was for me when I was starting out with react and gulp/Webpack two years ago.

I think the best option is the one you have described. I would like to preserve the existing classnames, ugly as they are, in order to reduce the effort needed to migrate. This will be a breaking change, obviously, and the fact that all users will be affected kind of makes my stomach churn. But it will be worth it in the long run, I suppose.

[brendanmoore]

@fritz-c You are right this would be a breaking change. Provided the user hasn't done to many of there own styling hacks it shouldn't be to ground shaking.

I remember when I started using webpack style-loader was a "magical mystery helper" - it was great - very useful, now that I think a lot of people just starting out with React are using CRA importing the css into your app is the new normal, and webpack 4 will have css-loader by default without the need for config.

Totally happy if you want to keep the classnames the same... So would you prefer this?

tree-node.js

// Currently
import React from "react";
import styles from "./tree-node.scss";

class TreeNode extends Component {
    /*...*/
    render() {
         /*...*/
        // styles.node hides the final classname from the user,
        // for devs who might not know webpack/css-modules
        // this could cause confusion 
        return (<div className={styles.node} {/*...*/} />);
    }
}

// Becomes
import React from "react"
import "./tree-node.scss"

class TreeNode extends Component {
    /*...*/
    render() {
         /*...*/
        return (<div className="rst__node" {/*...*/} />);
    }
}

tree-node.scss

/* Currrently */
.node {
  min-width: 100%;
  white-space: nowrap;
  position: relative;
  text-align: left;
}

/* Becomes */
.rst__node {
  min-width: 100%;
  white-space: nowrap;
  position: relative;
  text-align: left;
}

[fritz-c]

Yeah, that code is what I was talking about.

[arvinxx]

So what should I do if I want to wirtte overwirtte Less/SCSS when I use Webpack to compiler? I try it but find it doesn't work.

(I use less)

//SortableTree.less
.rst{
  &__moveHandle{
    height: 6px;
    width: 6px;
    border: solid #333 3px;
    cursor: pointer;
    border-radius: 50%;
    margin: 5px;
    z-index: 1;
  }
}

import React, { Component } from 'react';
import SortableTree from 'react-sortable-tree';
import 'react-sortable-tree/style.css'; // This only needs to be imported once in your app
import './SortableTree.less'

export default class Tree extends Component {
  constructor(props) {
    super(props);

    this.state = {
      treeData: [{ title: 'Chicken', children: [{ title: 'Egg' }] }],
    };
  }

  render() {
    return (
      <div style={{ height: 400 }}>
        <SortableTree
          treeData={this.state.treeData}
          onChange={treeData => this.setState({ treeData })}
        />
      </div>
    );
  }
}

Also I think about the hash name in webpack css-loader, and and more config to deal it.

         {
            test: /\.less$/,
            include: [/src\/components\/SortableTree/],
            use: [
              require.resolve('style-loader'),
              require.resolve('css-loader'),
              require.resolve('less-loader'),
              {
                loader: require.resolve('postcss-loader'),
                options: {
                  // Necessary for external CSS imports to work
                  // https://github.com/facebookincubator/create-react-app/issues/2677
                  ident: 'postcss',
                  plugins: () => [
                    require('postcss-flexbugs-fixes'),
                    autoprefixer({
                      browsers: [
                        '>1%',
                        'last 4 versions',
                        'Firefox ESR',
                        'not ie < 9', // React doesn't support IE8 anyway
                      ],
                      flexbox: 'no-2009',
                    }),
                  ],
                },
              },
            ],
          },
          {
            test: /\.less$/,
            exclude: [/node_modules/,/src\/components\/SortableTree/],
            use: [
              require.resolve('style-loader'),
              {
                loader: require.resolve('css-loader'),
                options: {
                  importLoaders: 1,
                  modules: true,
                  localIdentName: '[name]__[local]___[hash:base64:5]',
                },
              },
              'less-loader',
              {
                loader: require.resolve('postcss-loader'),
                options: {
                  // Necessary for external CSS imports to work
                  // https://github.com/facebookincubator/create-react-app/issues/2677
                  ident: 'postcss',
                  plugins: () => [
                    require('postcss-flexbugs-fixes'),
                    autoprefixer({
                      browsers: [
                        '>1%',
                        'last 4 versions',
                        'Firefox ESR',
                        'not ie < 9', // React doesn't support IE8 anyway
                      ],
                      flexbox: 'no-2009',
                    }),
                  ],
                },
              },

But actually it doesn't work at all.

[arvinxx]

resovle it by using

path.resolve(__dirname, '../src/components/SortableTree'),