Skip to content

Feature/theme spreads #47

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

Merged
merged 13 commits into from
Feb 20, 2017

Conversation

raveclassic
Copy link
Contributor

@raveclassic raveclassic commented Feb 9, 2017

Hi @javivelasco!
Firstly, my congrats on getting to 2.0 major!

Now about this PR.
Currently it's only possible to construct complex themes from more than two modules by composing themeable function which is far from convenient. Taking ES6 spreads into account this PR provides possibility to pass more than two arguments to themeable:

const a = {
  test: 'a'
}
const b = {
  test: 'b'
}
const c = {
  test: 'foo',
  foo: 'foo'
}
const expected = {
  test: 'a b foo',
  foo: 'foo'
}
const result = themeable(a, b, c)
expect(result).toEqual(expected)

Also it contains some refactoring of themeable function as it started to grow with type-check conditionals and setting of default values which is all pretty hard to maintain.

All tests pass, so take a look!

Update: I have found some TS typings added quite recently. Also themr build step is simple as hell - just a babel run. So I'd like to suggest switching to TS completely to be fully consistent with these typings and to enforce some real type checking. This is just a suggestion and I could prepare a simple PR with all code rewritten to TS. What do you think?

@raveclassic
Copy link
Contributor Author

raveclassic commented Feb 14, 2017

@TheSBros Hi, I've changed your initial typing of themr decorator to accept SFC and ComponentClass directly. I can't say for sure if I haven't broken something as it perfectly fits in my codebase instead of component: new(props?: P, context?: any) => React.Component<P, S> expression. Could you please review?

UPDATE: everything is ok with "@types/react": "^15.0.0"

@odensc
Copy link
Contributor

odensc commented Feb 14, 2017

@raveclassic If you look at #39 (comment) , there was a reason why I changed from ComponentClass.

It's currently late for me - but I will confirm if it still works in that situation tomorrow.

@raveclassic
Copy link
Contributor Author

@TheSBros I see the problem now. Will try to handle it somehow.

@raveclassic
Copy link
Contributor Author

raveclassic commented Feb 14, 2017

I've managed to solve both problems by adding a second call signature type to decorator interface. Check it out.

@raveclassic
Copy link
Contributor Author

Ok, I'm reverting typings changes because they are not related to initial PR. Only the part with themeable function is left and themr identifier can be a symbol now.

@raveclassic
Copy link
Contributor Author

I'm sorry about this mess with PR :)
Finally, this PR solves both issues with accepting sfc in themr and extending themeable interface

@javivelasco
Copy link
Owner

Thank you both! I think it makes total sense. I'm not very familiar with TS though. I want to get into it but I'm lacking time to do so. It's a very nice contribution, congrats :)

@javivelasco javivelasco merged commit 1586949 into javivelasco:master Feb 20, 2017
@raveclassic
Copy link
Contributor Author

@javivelasco Ok, I'll open a PR with a switch to TS when I have some free time. Surely build process migration will be completely transparent for you.

david-slayte pushed a commit to david-slayte/react-css-themr that referenced this pull request Jul 6, 2018
* refactoring

* theme spreads

* typings

* fix themr typing

* fix themr typing

* fix undefined mixin value cases

* fix state type incompatibility in javivelasco#39

* revert typing changes

* accept sfc as component

* typo

* also accept symbols and numbers as identifier

* merge sfc fix
palashkaria pushed a commit to locus-taxy/react-css-themr-legacy that referenced this pull request Jan 15, 2019
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

Successfully merging this pull request may close these issues.

3 participants