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

Rule proposal: no unused styles #2

Closed
lencioni opened this issue Aug 31, 2016 · 4 comments
Closed

Rule proposal: no unused styles #2

lencioni opened this issue Aug 31, 2016 · 4 comments

Comments

@lencioni
Copy link
Collaborator

All of the styles for a component should be self-contained within the component. This should make it pretty easy to identify styles that are defined but not actually used by the component, which would be a useful rule to have.

Bad:

function MyComponent({ styles }) {
  return (
    <div {...css(styles.foo)}>
      Foo
    </div>
  );
}

export default withStyles(() => ({
  foo: {
    backgroundColor: 'red',
  },

  bar: { // <--- this style is not used
    backgroundColor: 'green',
  },
}))(MyComponent);

Good:

function MyComponent({ styles }) {
  return (
    <div {...css(styles.foo)}>
      Foo
    </div>
  );
}

export default withStyles(() => ({
  foo: {
    backgroundColor: 'red',
  },
}))(MyComponent);
@ljharb
Copy link
Collaborator

ljharb commented Aug 31, 2016

This has a similar issue where it doesn't help when the HOC and the wrapped component are in separate files - which isn't an architecture we want to discourage, I'd think.

@lencioni
Copy link
Collaborator Author

When would that kind of architecture be desirable?

@ljharb
Copy link
Collaborator

ljharb commented Aug 31, 2016

Putting each thing in separate files? If you want to unit-test the individual component, if you want to shallow-render and avoid string selectors, if you want to use the component without the HOC for whatever reason… One component per file is generally preferred over consolidating imo.

@lencioni
Copy link
Collaborator Author

You can export the unwrapped component as a named export for those things. I agree that one component per file is best, but that usually excludes the use of higher order components. I don't think it would be a good idea to move the styles for a component to a separate file under most circumstances.

lencioni added a commit that referenced this issue Feb 17, 2017
There are a few edge cases that I decided to punt on for now. I tried to
note as many as I could think of in the documentation and in code
comments.

Fixes #2.
lencioni added a commit that referenced this issue Feb 17, 2017
There are a few edge cases that I decided to punt on for now. I tried to
note as many as I could think of in the documentation and in code
comments.

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

No branches or pull requests

2 participants