Open rybon opened 8 years ago
@rybon I found two issues with your Button functional component:
(props)
instead of ({ props })
<button className=...
instead of <button style=...
Maybe that already helps?
No, that was just some code I quickly wrote to illustrate the issue. I fixed the syntax. The issue remains though.
@rybon I can reproduce that issue now. I'm working on fixing it but it'll take some time. I'll keep you updated.
Great! Thank you for picking this up. I appreciate all the work you put into this library.
My idea for a possible fix:
Say my root React component is <App className={styles.app} />
. And I have <CustomButton className={styles.app.customButton} />
nested inside that component. Then the .app.
would cause the generated CSS to be something like:
.src_components_App_js-styles-app .src_components_CustomButton_js-styles-customButton { ... }
, which takes precedence over .src_components_Button_js-styles-button { ... }
.
Then, I can create application specific styles to override the generic styles of the generic <Button />
component.
I'm not sure whether this approach is a good or bad idea though.
I agree - Having a method of "netsting" classnames would allow for more specificity. As of right now, this generates an error, because "custombutton" is not one of the whitelisted pseudo elements that are allowed to be nested with objects:
const stylesheet = cssInJS({
theme: {
// Some styles here for general theming
custombutton: {
// styles for button here
// these would theoretically be available with className={stylesheet.theme.custombutton}
// and generate '.src_themes_js-stylesheet-theme .src_themes_js-stylesheet-theme-custombutton'
}
}
});
I think the main issue to figure out is the following: Is the object (that isn't a pseudo element) styling for specific subelements (i.e. .class div
), or is it a nested class for specificity (i.e. .class .class2
)?
I think the following syntax would make sense to distinguish between the two to allow for this functionality:
const stylesheet = cssInJS({
theme: {
// Some styles here for general theming
// Treat all nested objects that aren't pseudo elements as element styling within the class
// This generates the css rule .src_themes_js-stylesheet-theme div
div: {
},
// Treat all nested objects preceded by a '.' as a nested class for specificity purposes
// This generates the css rule '.src_themes_js-stylesheet-theme .src_themes_js-stylesheet-theme-custombutton'
'.custombutton': {
// styles for button here
// these would theoretically be available with className={stylesheet.theme.custombutton}
// and generate '.src_themes_js-stylesheet-theme .src_themes_js-stylesheet-theme-custombutton'
}
}
});
Thoughts?
I think the problem is that the ordering of styles in the generated bundle CSS file isn't correct. See https://jsfiddle.net/11qmaeu4/ for an example on how order of styles matters (you are probably aware of that).
The idea I have to fix this is to construct a dependency graph of components while the babel transformation happens (maybe something like that is already built-in into babel). Then I'll do a topological sort of those dependencies and write the components stylesheets in the resulting order into the bundle css.
For example, given we have the following component dependencies:
A --+--> B --+--> C
| |
| +--> D
|
+--> C
Sorting that results in
A --> B --> C --> D
which will be bundled as
D's styles
C's styles
B's styles
A's styles
Maybe that helps? WDYT?
I'm not quite sure that will work for all project structures. For example, assume the following project structure:
client/
configs/
themes/
index.js <-- This conglomerates all themes and allows importing them
theme1.js <-- This contains a cssInJS exported stylesheet
theme2.js <-- This contains another cssInJS exported stylesheet
modules/
core/
A.jsx <-- All of these components have their own cssInJS stylesheets
B.jsx <-- but also import classes from '/configs/themes'
C.jsx
D.jsx
In this case, how would you know where to stick the cssInJS generated classes into the bundle when the specificity of imported things might not be known? In other words, what if A, B, C, and D all depend on various classes from the imported stylesheet variables (from the themes directory)? How do those get ordered?
I tested this approach and got the styles to be generated in a reverse order, but that didn't seem to help as far as I could determine. I feel the solution is to provide an API that will make certain selectors more specific than others. In my case, nesting my theme classes under an 'app' class is enough. I suppose we'd have to walk the component tree starting from the root, analyse every cssInJS
object for top-level key names pointing to a parent cssInJS
object, and prepend that parent class in the resulting selector.
What about introducing a special css rule that acts as a directive to prepend something to the generated classname? Example:
const PrimaryButton = () => (<Button className={styles.primary}>Click me!</Button>);
const styles = cssInJS({
primary: {
'@prepend': '.my-theme',
fontSize: 24
}
});
The above will be transformed to:
const PrimaryButton = () => (<Button className={styles.primary}>Click me!</Button>);
const styles = {
primary: 'PrimaryButton_js-styles-primary'
};
And the generated CSS will be:
.my-theme .PrimaryButton_js-styles-primary {
font-size: 24px;
}
Sure, that could work. The only thing that feels a bit off with that approach is that we deviate from standard CSS syntax. But fine as a fix for now I suppose.
Is the @prepend
syntax easier to support than allowing nested classes? What if this generated the same CSS as the @prepend
syntax?
const styles = cssInJS({
mytheme: {
primary: {
fontSize: 24
}
}
});
To me, this feels more intuitive and is more expressive in the code for use (i.e. className={stylesheet.mytheme.primary}
) -- this suggests a prepended class. If I just access primary
that has a @prepend
, then the code is not expressive in suggesting that there is prepended classes (i.e. className={stylesheet.primary}
).
Oh I see what your main use case was. Is the idea that .my-theme
is defined somewhere and then the @prepend
would be defined somewhere else?
Here's a crazy idea that might solve all use cases. css-in-js
could provide a special version of classnames
that would build a precise “CSS dependency graph” between components and styles, and use that to sort CSS in the output.
E.g.:
const OuterComponent = () =>
<InnerComponent className={classNames(outerStyles.redBorder)} />;
const InnerComponent = ({className}) =>
<div className={classNames(className, innerStyles.blueBorder)} />;
While parsing this, when we encounter the first instance of classNames
, we detect that OuterComponent
applies outerStyles
to InnerComponent
. This builds a graph like this:
OuterComponent → outerStyles → InnerComponent
When we encounter the second classNames
, the graph becomes:
OuterComponent → outerStyles → InnerComponent → innerStyles → div
Once parsing is done, this boils down to:
outerStyles → innerStyles
So we know to sort outerStyles
after innerStyles
.
I think an approach like this is a more general and more robust solution than having to manually increase the specificity of the selectors. Thoughts?
We could even take this one step further and consider the order of the styles inside the classNames
call, and give precedence to the styles applied later. This would be very powerful, and allow control previously only possible with inline styles.
For example:
const InnerComponent = ({className}) =>
<div className={classNames(innerStyles.defaultBorder, className, innerStyles.padding)} />
The intent here is that the border can be overridden, but the padding can’t. This can be achieved with a graph like this:
innerStyles.padding ↠ InnerComponent → innerStyles.defaultBorder → div
↑
OuterComponent → outerStyles.redBorder
That would be transformed into:
innerStyles.padding → innerStyles.defaultBorder
↓ ↑
outerStyles.redBorder
Which results in:
innerStyles.defaultBorder
outerStyles.redBorder
innerStyles.padding
Implementation could be tricky, but it seems doable.
Great suggestions! Should be doable indeed. Maybe the PostCSS toolchain can help us out here.
css-in-js
could provide a special version ofclassnames
that would build a precise “CSS dependency graph” between components and styles, and use that to sort CSS in the output.
Isn't that what the classnames package already does?
The JS API of the provided classnames
would be the same, yes. But the existing classnames
does nothing special to help the output of css-in-js
styles. The proposed classnames
would be special in that it would serve as a marker for the preprocessor (sort of like cssInJS()
).
The preprocessor needs something to look for to detect when classes are applied to other components. I guess it could also just look for all occurrences of the className
attribute, but I thought an explicit function call would make preprocessing easier.
All generated classes have the same specificity, but that complicates extending generic styles with custom styles for a specific theme.
Example:
When I render
CustomButton
, thecolor: 'blue'
still takes precedence overcolor: 'green'
, but I want my green theming to override the default styling. Is there a mechanism to do that without resorting to specificity hacks?