Theme support: more flexible custom class names and support for inline styles

[kof]

Can we make global class names optional? There are a number of ways, for e.g. passing a theme object like react-theamable suggests.

I am using jss and would like to have consistency in class names.

[bvaughn]

Hey @kof,

Styling is kind of a difficult issue for libraries like RV because I don't think the community has really embraced a standard way of doing things. I don't think it's possible for RV to support everyone's styling approach (because they're too varied) so I tried to pick one that's the most flexible and open. A good bit of thought and discussion went into it (see issues like #48). The benefits are:

• Supports universal applications (ie. works well for browser and Node, something an earlier version of inline styles did not do due to its auto-prefixer).
• Customizable on a per-element basis as well as entirely (via a fork of the stylesheet).
• Doesn't add much clutter or complexity to RV library code.

That being said I realize it also has some downsides. I'd like to avoid using global class names because of their potential for conflict. I'd also (personally) prefer to use inline styles so that users didn't have to remember to include a stylesheet during their bootstrapping process. (People forget to do this pretty frequently.) But inline styles have priority over external styles and so I avoid them because it would make customization more difficult for RV users who didn't also embrace inline styles. (As I mentioned above it has also caused some complications WRT auto-prefixing in the past.)

RV also allows you to supply your own class names for elements (to be appended to the classnames list) so this should hopefully allow you to use your own approach to styling for customization purposes at least somewhat?

Happy to discuss this more.

[kof]

I think there is a universal way which will work for every one: allow components to accept a theme object which is a map of names and class names:

const theme = {
   button: 'my-button'
}
<List theme={theme} />

This allows to have global class names by default as it is right now and at the same time overwrite everything by a user.

[kof]

RV also allows you to supply your own class names for elements

yes, but not to all of them and global classes are still a problem then.

[bvaughn]

Sure, that's true. I only added custom className support for what I saw as the biggest value components to avoid props-explosion.

You say global classes are "a problem" but... why? If you're using JSS or CSS-modules their presence shouldn't affect you since both allow you to namespace your application styles.

I don't want to go the route of react-theamable in this case. I think it works well for a whitelist of styles (eg. specific colors or font sizes you want to make customizable) but not for all of the styles a library like react-virtualized needs (eg. flex, overflow, etc).

Your suggestion of a parameter that specifies classNames is potentially something I could do. But I think it has a couple of potential downsides (though minor):

• I wouldn't want to support a third option for customizing styles so this sort of change would probably have to wait until the next major release version.
• It seems ambiguous what should happen if a property (class name) is missing from this param. Should RV fallback to its default class name? Should it not attach any class names at all to the element in question?
• I don't want to make simple customizations more difficult. For example, I think it's currently common for users to want to customize a single attribute or a grid (eg. background color, border style) but otherwise stick with the default styles. With the approach you describe, I think users would have specify a lot of boilerplate names just to override a specific style. Granted there are ways to reduce that boilerplate but it seems like something the current approach (additive class names) avoids nicely.

[kof]

You say global classes are "a problem" but... why? If you're using JSS or CSS-modules their presence shouldn't affect you since both allow you to namespace your application styles.

Imagine a case where external components or widgets are used at the same time. You can never know what class names they use. Also it's just not nice to have them around when there is an easy way to get rid of them.

Regarding breaking changes: I totally see we can do it very nice and without breaking changes.

Theme is an object with a given interface which can be validated. If user passes a theme object he will pass all required props. If he doesn't pass theme object - also fine, global names as now are used in default theme object and we expect the style sheet loaded to the document by the user exactly as it is now. In those cases where user can pass a class name - nothing changes, theme is theme, it is more for static parts, className is something that one might want to change at render time. No conflicts there and also everything is good to have.

[bvaughn]

I understand that there's potential for conflicting class names. That's a limitation of static class names, yup. On the other hand, it's already broadly in use and (up until this point) I haven't heard of any conflicts. So it seems a little bit more like a conceptual problem than a practical one.

I don't mean to write off your observation that it's not ideal. I love CSS modules for what it's worth because they avoid this problem. I guess I'm just pointing out that... in my opinion... the potential downside of static class names needs to be weighed against:

1. The convenience for users when it comes to overriding one-off styles and...
2. My personal time when it comes to supporting a variety of different ways to customize and style a component; (more ways means more possibility bugs and more maintenance effort).

I'm afraid I don't understand your proposal. It was a long day (at work) so maybe I'm just mis-reading it but could you try to restate what you're suggesting?

[kof]

Sorry, its probably my bad in explaining those ideas. I will prototype something so that you can see it in code.

[kof]

I have created a pull request on another project regarding theming, we can do here exactly the same https://github.com/elementalui/elemental/pull/150/files

[bvaughn]

Hi @kof.

Thanks for the update. Read your PR and the discussion attached, and the spec you linked to. It all looks pretty solid.

I have a concern for react-virtualized usage though. I mentioned it before but it will be easier to talk about now that I can see code. With the PR you linked to, I think it would be more work to do small style modifications (eg. a custom background color or border) because a user would need to duplicate an entire theme object (including all of the classes/states they don't care about customizing).

I think this concern could be reduced by exposing some sort of theme-creation factory function. I added some thoughts on the other PR for one way we could address that concern.

[bvaughn]

I realize this morning that it's perhaps a little ambiguous to anyone seeing only the discussion on this issue- but I've decided to move forward with the theming approach we're discussing (or at least one similar) as part of the version-7 update that I'm currently working on.

[kof]

Can we collaborate on theme-standard at the same time? Means adding docs there and defining the workflow for theming in general.

[bvaughn]

@kof I'm not sure if the idea of supporting "themeable" things like colors, fonts, etc. really applies to react-virtualized. I was considering for inspiration something like alexkuz/react-json-tree minus the base16 theme bit since react-virtualized doesn't really have colors.

What did you have in mind in terms of collaboration though? I'm a fan of collaborating in general.

PS. When can I see more of your dynamic height spike? ;)