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

warn users when allegedly "themed" styles dont use themes. #1005

Closed
iamstarkov opened this issue Jan 29, 2019 · 10 comments
Closed

warn users when allegedly "themed" styles dont use themes. #1005

iamstarkov opened this issue Jan 29, 2019 · 10 comments
Labels
complexity:low You can fix it, c'mon! enhancement No kittens die if we don't do that.

Comments

@iamstarkov
Copy link
Member

Is your feature request related to a problem? Please describe.

Sometimes developers define styles as themed once purely out of habit without real necessity to depend on a theme.

export const styles = () => ({
  root: {},
});

Apparently people do it a lot.

Describe the solution you'd like
When component pulls this trick please warn developer this component doesn't need to use themed styles.

Are you willing to implement it?
Yes

@HenriBeck
Copy link
Member

You can do it with fn.length:

function styles(test) {}

console.log(styles.length);

https://codesandbox.io/s/zx0l4p527p

@kof
Copy link
Member

kof commented Jan 29, 2019

Thats quite easy to add.

@kof kof added enhancement No kittens die if we don't do that. complexity:low You can fix it, c'mon! labels Jan 29, 2019
@HenriBeck
Copy link
Member

We could even do the same with function styles in development so that they also only receive one argument.

We should warn when the user uses anything other than one argument.

@kof
Copy link
Member

kof commented Jan 29, 2019

Yeah

@iamstarkov
Copy link
Member Author

is master branch a proper branch to base PR on?

@HenriBeck
Copy link
Member

yes

@iamstarkov
Copy link
Member Author

I'm on it then

@iamstarkov
Copy link
Member Author

offtopic question. why author field in react-css is the way it is?

   "author": {
    "name": "Dan Abramov",
    "email": "[email protected]"
  },

@kof
Copy link
Member

kof commented Jan 29, 2019

@gaearon has originally created the first version of react-jss. I am not sure what the traditions are around author field, it has been obviously rewritten several times since then.

@HenriBeck
Copy link
Member

I think we should make the author field something which points to our repo and contributors.

Also, it should be the same for every package in my opinion.

HenriBeck pushed a commit that referenced this issue Feb 3, 2019
…te-sheets-manager

* 'master' of https://github.com/cssinjs/jss:
  warn consumers if themed styles are misused. fix #1005 (#1006)
  Added TypeScript Usage documentation for React JSS (#1009)
HenriBeck pushed a commit that referenced this issue Feb 12, 2019
… update-jss-types

* jss-plugin-rule-value-function/fix-process-styles: (35 commits)
  Add formatting TS files
  Update typings
  Use types jss definitions
  Fix TS type definitions (#1030)
  v10.0.0-alpha.10
  Update publish scripts (#1027)
  [docs] Update SSR setup with react-jss (#1018)
  temporarily remove the link to the docs, we need perma links instead #1006
  [jss] Update SheetsManager (#1019)
  warn consumers if themed styles are misused. fix #1005 (#1006)
  Added TypeScript Usage documentation for React JSS (#1009)
  [react-jss] Export the context as __Context from react-jss (#1014)
  [jss-plugin-camel-case] Support css vars better (#1017)
  [docs] Add badges to readmes (#1016)
  Revert "Add tests for css variables in camel case plugins"
  Revert "Fix css variables being hyphenated"
  Revert "Run prettier"
  Run prettier
  Fix css variables being hyphenated
  Add tests for css variables in camel case plugins
  ...

# Conflicts:
#	packages/jss/src/index.d.ts
bhupinderbola pushed a commit to bhupinderbola/jss that referenced this issue Sep 17, 2019
…ssinjs#1006)

* warn consumers if themed styles are misused. fix cssinjs#1005

* Fix tests on node 10

upgrade karma-browserstack-launcher to at least 1.4.0,
because "internalBinding is not defined" due to old "natives" module
See more:
* karma-runner/karma-browserstack-launcher#140
* popcodeorg/popcode#1626

* add tests

* update snapshots

* add test for themed styles misuse warning not shown in prod

* Add dev expression babel plugin while testing

* Remove test and babel plugin from test setup

* Update size-snapshot
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
complexity:low You can fix it, c'mon! enhancement No kittens die if we don't do that.
Projects
None yet
Development

No branches or pull requests

3 participants