Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Question about syntax #114

Open
VladimirZaets opened this issue Oct 18, 2017 · 9 comments
Open

Question about syntax #114

VladimirZaets opened this issue Oct 18, 2017 · 9 comments

Comments

@VladimirZaets
Copy link

VladimirZaets commented Oct 18, 2017

Hi, Yesterday I took your package to replace withStyles from material-ui for yours.
I have a question about construction
<div ...css(styles.classname1, styles.classname2)/>
As for me that syntax is slightly redundant and also with that I can't use "classname" package, as example:
{ classnames(css(styles.classname1)), {classnames(css(styles.classname2)): this.state.error } }, or simply add some static styles, as example:
<div className="error", {...css(styles.classname)}> or
<div className={classname({ classnames(css(styles.classname1)), {error: this.state.error})}.

It's only two cases that I've run into in a few days.
I've wrapped some methods and now it works as always, as example:
<div className={classnames(styles.firstLink, styles.secondLink, {error: true})}>
That's work with JSS and Aphrodite.

Methods that I wrap:

const cssWrapper = (...params) => css.apply(css, params).className;
const aphroditeInterfaceCustom = {
    ...aphroditeInterface,
    create (styleHash) {
        const styles = aphroditeInterface.create(styleHash);

        return each(styles, (value, key) => {
            styles[key] = cssWrapper(value);
        });
    }
};

So, maybe I don't know or missed something, please explain for me why you chose this approach.

@ljharb
Copy link
Collaborator

ljharb commented Oct 19, 2017

The idea is that you don't use class names or inline styles at all - you only style a component with withStyles. ...css() is useful because that means the interface can decide if it's using classNames, or inline styles, to style the component.

@VladimirZaets
Copy link
Author

VladimirZaets commented Oct 19, 2017

I understand the main idea, but for example I have the real workcase.
I have <div> element and I want to set first class permanently and the second class dynamically.
Before I did that:
<div className={classname(style.myFirstClass, {[styles.mySecondClass]: condition})} /> What I should do with yours approach ?
I have only one idea:
<div { condition ? ...css(style.myFirstClass, style.mySecondClass) : ...css(style.myFirstClass) } /> and in cases where I have a lot of permanent classes I should always duplicate them?

@ljharb
Copy link
Collaborator

ljharb commented Oct 20, 2017

<div
  {...css(
    style.myFirstClass,
    condition && style.mySecondClass,
  )}
/>

@chenasraf
Copy link

I'll go ahead and add that for incremental migration, this causes a problem - you can't mix regular CSS classes with withStyles just as an override pattern. I agree that there should be at least a way to extract class names easily or to mix them with non-withStyles classes in the css method.

@ljharb
Copy link
Collaborator

ljharb commented Jun 4, 2018

@chenasraf indeed, incremental migration is only possible when your component is fully self-contained - my suggestion would be to start with a leaf, fix all places where it’s being overridden with css and change them to use props, and then you can migrate that leaf to react-with-styles.

@Nazerbayev
Copy link

Nazerbayev commented Jul 24, 2018

I do agree with The idea is that you don't use class names or inline styles at all, the problem is for example if you are developing a component that will be used as a dependency on other projects. Ideally the component should be flexible enough to allow the users to override your styles, and the easiest way to do that is having one or more props with custom static classnames defined by the user, not the component developer.

Edit: I dont know if adding external static classnames via props is currently possible, at least it is not clear enough how to do that on the current documentation.

@ljharb
Copy link
Collaborator

ljharb commented Jul 24, 2018

Ideally the component should be flexible enough to allow the users to override your styles

Our answer to that is theming, via react-with-styles.

@Nazerbayev
Copy link

Nazerbayev commented Jul 24, 2018

So, if I want to make a shareable component and I want it to be flexible allowing the other party to add their own classnames in the inner pieces of my code then the solution is to make the other party add an additional dependency to their project (and learn how to use it) instead of using the usual pattern of a simple prop?

I apologize if im not understanding something basic, but it seems to me that it goes totally agaisnt the main premise of this library, citing from the very first line of the readme: components without being tightly coupled to one implementation.

Edit: I want to clarify something, i'm not complaining for the sake of complaining, my case is that i'm reading through the source code of react-dates looking for a way to override some style choices that it has, probably making a fork if i have to, and I stumbled upon this library.

@ljharb
Copy link
Collaborator

ljharb commented Jul 24, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants